Rename group' to groupAll and add groupAllBy; add docs #200
Conversation
|
CI passes. Ready for review. |
|
This applies to #194 too but I wonder if it's worth considering dropping the |
|
Perhaps a variant called groupAllBy' :: forall a. (a -> a -> Boolean) -> (a -> a -> Ordering) -> Array a -> Array (NonEmptyArray a)
groupAllBy' p c = groupBy p <<< sortBy cDoes this pass the Fairbairn threshold? |
Meaning this? groupAllBy :: forall a. (a -> a -> Ordering) -> Array a -> Array (NonEmptyArray a)
groupAllBy cmp = groupBy (eq Eq $ cmp) <<< sortBy cmp |
|
Something like the following should work: groupAllBy comp = groupBy (\x y -> comp x y == EQ) <<< sortBy comp |
| -- | groupAllBy (\a b -> odd a && odd b) [1, 3, 2, 4, 3, 3] | ||
| -- | = [NonEmptyArray [1], NonEmptyArray [2], NonEmptyArray [3, 3, 3], NonEmptyArray [4]] | ||
| -- | groupAllBy (\a b -> compare (odd a) (odd b)) [1, 3, 2, 4, 3, 3] | ||
| -- | = [NonEmptyArray [2, 4], NonEmptyArray [1, 3, 3, 3]] | ||
| -- | ``` |
There was a problem hiding this comment.
I adapted the example to the one above, but I don't think it's a good example. Any ideas?
There was a problem hiding this comment.
What about using comparing Down?
groupAllBy (comparing Down) [1, 3, 2, 4, 3, 3]
= [NonEmptyArray [4], NonEmptyArray [3, 3, 3], NonEmptyArray [2], NonEmptyArray [1]]
| @@ -987,15 +987,15 @@ groupBy op xs = | |||
| STA.unsafeFreeze result | |||
|
|
|||
| -- | Group equal elements of an array into arrays, using the specified | |||
| -- | equivalence relation to determine equality. | |||
| -- | comparison relation to determine equality. | |||
| -- | | |||
There was a problem hiding this comment.
How should docs be updated to better communicate what this does?
There was a problem hiding this comment.
I think we could replace “comparison relation” by “partial ordering“, as in the documentation of sortBy, here.
There was a problem hiding this comment.
Those docs are actually incorrect - it's expecting a total ordering, not a partial one. I think they must be an oversight left over from a short period in the distant past where we said Ord represented a partial ordering rather than a total ordering. We call this a "comparison function" in Data.Array.ST, and we say "where elements are compared according to the specified ordering" in Data.List. I think either of those are fine.
There was a problem hiding this comment.
I've reverted back to using comparison function
| @@ -987,15 +987,15 @@ groupBy op xs = | |||
| STA.unsafeFreeze result | |||
|
|
|||
| -- | Group equal elements of an array into arrays, using the specified | |||
| -- | equivalence relation to determine equality. | |||
| -- | comparison relation to determine equality. | |||
| -- | | |||
There was a problem hiding this comment.
I think we could replace “comparison relation” by “partial ordering“, as in the documentation of sortBy, here.
| -- | groupAllBy (\a b -> odd a && odd b) [1, 3, 2, 4, 3, 3] | ||
| -- | = [NonEmptyArray [1], NonEmptyArray [2], NonEmptyArray [3, 3, 3], NonEmptyArray [4]] | ||
| -- | groupAllBy (\a b -> compare (odd a) (odd b)) [1, 3, 2, 4, 3, 3] | ||
| -- | = [NonEmptyArray [2, 4], NonEmptyArray [1, 3, 3, 3]] | ||
| -- | ``` |
There was a problem hiding this comment.
What about using comparing Down?
groupAllBy (comparing Down) [1, 3, 2, 4, 3, 3]
= [NonEmptyArray [4], NonEmptyArray [3, 3, 3], NonEmptyArray [2], NonEmptyArray [1]]
test/Test/Data/Array.purs
Outdated
| @@ -363,10 +363,10 @@ testArray = do | |||
| assert $ A.groupBy (\_ _ -> true) [1, 2, 3] == [nea [1, 2, 3]] | |||
|
|
|||
| log "groupAllBy should group equal elements into arrays based on an equivalence relation" | |||
There was a problem hiding this comment.
What about “based on the result of a comparison function”, as in the test for sortBy?
|
I've addresses all your comments. |
|
This is ready for another look over. |
Addresses the comments that @kl0tli made here