-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Implement dropRepeatsBy (#3041) #3239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for a minor nit in the tests, I have no issues with how this is written.
But I'm still not convinced of the need for it. Is there any good argument for including it besides the (important, but never crucial) consistency of a parallel for dropRepeatsWith
.
Have you had common needs for such a function?
EDIT: Requested changes have been addressed.
There certainly are real-world scenarios for this function. Last year, when I created #3041, my use case was the following: given a pageview history page, I needed to filter out duplicate visits from the same user, displaying only the latest one. IMHO this would be the simplest form: const visits = [
{
date: '2021-12-02',
userId: 1
},
{
date: '2021-12-01',
userId: 1
},
{
date: '2021-11-27',
userId: 2
},
{
date: '2021-11-25',
userId: 1
}
]
const uniqueVisits = R.dropRepeatsBy(R.prop('userId'), visits)
console.log(uniqueVisits)
/*
Output:
[
{
date: '2021-12-02',
userId: 1
},
{
date: '2021-11-27',
userId: 2
},
{
date: '2021-11-25',
userId: 1
}
]
*/ TL;DR: Use case is similar to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a while to decide, but I'm in favor of this function. The consistency is nice, and it's genuinely useful. A custom implementation is not difficult, but neither is it trivial.
🌿
Thank you Ivan for your contribution, and for your patience! |
I hit "Close" rather than "Merge". Oops! Fixed now. |
I've seen it LOL Thanks for merging. I'm sure it'll be useful for the community. |
see discussion here