-
Notifications
You must be signed in to change notification settings - Fork 70
Rename group' to groupAll and add groupAllBy; add docs #200
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
Merged
thomashoneyman
merged 6 commits into
purescript:master
from
JordanMartinez:addNEAGroupFunctions
Dec 29, 2020
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
33975f1
Rename group' to groupAll and add groupAllBy; add docs
JordanMartinez f6abfae
Replace implementation's deprecated `group'` with `groupAll`
JordanMartinez e489ea1
Quote `groupAll` in deprecation warning
JordanMartinez b6c34d9
Update `groupAllBy` to use `compare` for both sorting and grouping
JordanMartinez 717446a
Address feedback: update docs and use `comparing Down` as example
JordanMartinez 9ed75eb
Revert back to 'comparison function' phrasing for docs
JordanMartinez File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -959,10 +959,10 @@ group xs = groupBy eq xs | |
-- | groupAll [1, 1, 2, 2, 1] == [NonEmptyArray [1, 1, 1], NonEmptyArray [2, 2]] | ||
-- | ``` | ||
groupAll :: forall a. Ord a => Array a -> Array (NonEmptyArray a) | ||
groupAll = groupAllBy eq | ||
groupAll = groupAllBy compare | ||
|
||
-- | Deprecated previous name of `groupAll`. | ||
group' :: forall a. Warn (Text "'group\'' is deprecated, use groupAll instead") => Ord a => Array a -> Array (NonEmptyArray a) | ||
group' :: forall a. Warn (Text "'group\'' is deprecated, use 'groupAll' instead") => Ord a => Array a -> Array (NonEmptyArray a) | ||
group' = groupAll | ||
|
||
-- | Group equal, consecutive elements of an array into arrays, using the | ||
|
@@ -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 function to determine equality. | ||
-- | | ||
-- | ```purescript | ||
-- | groupAllBy (\a b -> odd a && odd b) [1, 3, 2, 4, 3, 3] | ||
-- | = [NonEmptyArray [1], NonEmptyArray [2], NonEmptyArray [3, 3, 3], NonEmptyArray [4]] | ||
-- | groupAllBy (comparing Down) [1, 3, 2, 4, 3, 3] | ||
-- | = [NonEmptyArray [4], NonEmptyArray [3, 3, 3], NonEmptyArray [2], NonEmptyArray [1]] | ||
-- | ``` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about using groupAllBy (comparing Down) [1, 3, 2, 4, 3, 3]
= [NonEmptyArray [4], NonEmptyArray [3, 3, 3], NonEmptyArray [2], NonEmptyArray [1]] |
||
-- | | ||
groupAllBy :: forall a. Ord a => (a -> a -> Boolean) -> Array a -> Array (NonEmptyArray a) | ||
groupAllBy p = groupBy p <<< sort | ||
groupAllBy :: forall a. (a -> a -> Ordering) -> Array a -> Array (NonEmptyArray a) | ||
groupAllBy cmp = groupBy (\x y -> cmp x y == EQ) <<< sortBy cmp | ||
|
||
-- | Remove the duplicates from an array, creating a new array. | ||
-- | | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How should docs be updated to better communicate what this does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could replace “comparison relation” by “partial ordering“, as in the documentation of sortBy, here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reverted back to using
comparison function