Skip to content
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

Implement dropRepeatsBy (#3041) #3239

Merged
merged 4 commits into from Feb 23, 2022

Conversation

ivanfilhoz
Copy link
Contributor

see discussion here

Copy link
Member

@CrossEye CrossEye left a 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?

test/dropRepeatsBy.js Outdated Show resolved Hide resolved
@ivanfilhoz
Copy link
Contributor Author

ivanfilhoz commented Feb 22, 2022

Except for a minor nit in the tests, I have no issues with how this is written.

EDIT: Requested changes have been addressed.

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?

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 R.uniqBy, but only applies to contiguous equalities, just like R.uniq vs R.dropRepeats.

source/dropRepeatsBy.js Outdated Show resolved Hide resolved
test/dropRepeatsBy.js Show resolved Hide resolved
Copy link
Member

@CrossEye CrossEye left a 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.

🌿

@CrossEye
Copy link
Member

Thank you Ivan for your contribution, and for your patience!

@CrossEye CrossEye closed this Feb 23, 2022
@CrossEye CrossEye reopened this Feb 23, 2022
@CrossEye CrossEye merged commit c7aef9a into ramda:master Feb 23, 2022
@CrossEye
Copy link
Member

I hit "Close" rather than "Merge". Oops! Fixed now.

@ivanfilhoz
Copy link
Contributor Author

I've seen it LOL

Thanks for merging. I'm sure it'll be useful for the community.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants