Revise groupAllBy to just use an Ordering function #191
Conversation
|
Can you please include only the changes for groupAllBy in this PR, and not the other documentation changes? |
| log "groupAllBy should group equal elements into lists based on an equivalence relation" | ||
| assert $ groupAllBy (\x y -> odd x && odd y) (l [1, 3, 2, 4, 3, 3]) == l [NEL.singleton 1, NEL.singleton 2, NEL.NonEmptyList (3 :| l [3, 3]), NEL.singleton 4] | ||
| log "groupAllBy should sort then group equal elements into lists based on a comparison function" | ||
| assert $ groupAllBy (compare `on` (_ `div` 10)) (l [32, 31, 21, 22, 11, 33]) == l [nel 11 [], nel 21 [22], nel 32 [31, 33]] |
There was a problem hiding this comment.
A test case like this is necessary to demonstrate the degree of ordering preserved.
I wanted this to be consistent with the docs and then I wanted to make sure this was easily relatable to the other group* tests and examples.
src/Data/List.purs
Outdated
| -- | For example, | ||
| -- | |
There was a problem hiding this comment.
I think it's pretty obvious that what follows is an example, so figured it would be better to remove that line to make the pursuit results more compact.
src/Data/List.purs
Outdated
| -- | | ||
| -- | For example, | ||
| -- | Group equal, consecutive elements of a list into lists, using the provided | ||
| -- | equivalence function to determine equality. |
There was a problem hiding this comment.
This is the approved phrasing for the nubBy changes, so wanted to keep things consistent here too.
|
I can thin down this PR so it only touches groupAllBy and then make a follow-up PR with the docs consistency changes. Ideally, those could be merged back-to-back. |
|
Yes please, these two things should ideally be reviewed separately. |
This PR was already listed
|
Resolved latest merge conflicts. Ready for another review. |
| - Added `groupAllBy` (#182, #191) | ||
| - Added `groupAllBy` (#182) |
There was a problem hiding this comment.
I prematurely added this PR to the changelog in #195. I was hoping this PR would have been included in the batch of breaking changes, so the release history would show groupAllBy as being added correctly the first time.
In hindsight, it would have been better to add the #191 tag here in this PR. That could have achieved the same result (assuming it got merged before the release), and would have avoided recent confusion where rebasing to master eliminated all changelog edits for this PR.
|
@JordanMartinez Please add "breaking change" label. |
JordanMartinez
added
the
type: breaking change
|
@milesfrain Done thanks! |
JordanMartinez
added
the
purs-0.15
Description of the change
Fixes #189
Also modified the tests and examples to demonstrate which grouping functions involve sorting.
One of the commits reverts demoing the convenience of the
:||infix operator proposed in #190. We can redo those changes if that operator is accepted.Checklist: