-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix constraints and names for new Foldable methods #3122
Fix constraints and names for new Foldable methods #3122
Conversation
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.
Good catch 👍
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.
Thanks!
Codecov Report
@@ Coverage Diff @@
## master #3122 +/- ##
=======================================
Coverage 93.17% 93.17%
=======================================
Files 372 372
Lines 7182 7182
Branches 198 198
=======================================
Hits 6692 6692
Misses 490 490
Continue to review full report at Codecov.
|
Ohh, I think I see what happened here. Initially added them as bincompat syntax I think, then realized we can have them in the typeclasses after dropping 2.11, but I completely forgot to remove the constraints.
Regardless, thank you @travisbrown! I'm ok with the name change as well. |
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.
Thank you!
@joroKr21 Ah, that makes sense! |
This is a follow-up for #3084, with three changes, two of which should be uncontroversial:
Foldable
andReducible
type classes, and shouldn't haveFoldable
orReducible
constraints, so I've removed them.@see
clauses in the API docs for theFoldable
methods (they should point to the*By
methods onReducible
).I've also changed the method names from
minimumOptionBy
andmaximumOptionBy
tominimumByOption
andmaximumByOption
. This is consistent with the standard library'smaxByOption
, and also with e.g.reduceRightOption
(from both the standard library and Cats).Following the standard library here seems like the right thing to do, and these new methods haven't been in a release yet, so we don't need to worry about compatibility. If people would prefer to keep the
OptionBy
versions I can remove that commit, though./cc @joroKr21