purescript/purescript-lists

Revise groupAllBy to just use an Ordering function

milesfrain opened this issue · 3 comments

milesfrain commented

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 commented

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

milesfrain commented

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

  • 👍1
JordanMartinez commented

Thanks Miles!