Skip to content

Revise groupAllBy to just use an Ordering function #189

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

Closed
milesfrain opened this issue Jan 12, 2021 · 3 comments · Fixed by #191
Closed

Revise groupAllBy to just use an Ordering function #189

milesfrain opened this issue Jan 12, 2021 · 3 comments · Fixed by #191
Assignees
Labels
type: enhancement A new feature or addition.

Comments

@milesfrain
Copy link
Contributor

Proposing we modify the newly-added (in #182) groupAllBy from:

groupAllBy :: forall a. Ord a => (a -> a -> Boolean) -> List a -> List (NEL.NonEmptyList a)

to:

groupAllBy :: forall a. (a -> a -> Ordering) -> List a -> List (NEL.NonEmptyList a)

The existing version can lead to undesired output where the Ord instance fights against your clustering rule.
The new version also more closely matches the pattern set by these other functions:

sortBy :: forall a. (a -> a -> Ordering) -> List a -> List a
-- (the below is true after #179)
nubBy :: forall a. (a -> a -> Ordering) -> List a -> List a

If we do this before 0.14, we don't even have to worry about an API bump.

@kl0tl for review.

@hdgarrood
Copy link
Contributor

Definitely 👍 we made this change already for the same reason in arrays, I think.

@milesfrain
Copy link
Contributor Author

FYI, I'm working on making this change. Don't want to duplicate efforts.

@JordanMartinez
Copy link
Contributor

Thanks Miles!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A new feature or addition.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants