Add max/min and reduceOption methods #1167
Conversation
This adds: - `reduceLeftOption` and `reduceRightOption` methods to `Foldable` - `maximumOption` and `minimumOption` methods to `Foldable` - `maximum` and `minimum` methods to `Reducible` Resolves typelevel#1164.
Current coverage is 88.98%@@ master #1167 diff @@
==========================================
Files 232 232
Lines 3073 3078 +5
Methods 3021 3025 +4
Messages 0 0
Branches 49 50 +1
==========================================
+ Hits 2729 2739 +10
+ Misses 344 339 -5
Partials 0 0
|
|
👍 thanks! |
| @@ -112,6 +112,12 @@ import simulacrum.typeclass | |||
| val F = self | |||
| val G = Reducible[G] | |||
| } | |||
|
|
|||
| def minimum[A](fa: F[A])(implicit A: Order[A]): A = | |||
There was a problem hiding this comment.
why not .min to be closer to stdlib (and fewer chars).
There was a problem hiding this comment.
I did this specifically to avoid collision with the std lib. If I refactor something from a NonEmptyList to a List I'd prefer to get a compile error instead of something with no type signature change but that will start throwing exceptions at runtime.
Having said that, there's a question of how much we should bend over backwards to avoid collisions with std lib names. I'm not set in stone on this if people would prefer min.
There was a problem hiding this comment.
In this case you would have Reducible[NonEmptyList].min(myThing) how could there be a collision with the stdlib in practice? The collision could come with an implicit class that adds .min to any Reducible, and then we have anxiety, perhaps, when reading the code if we are getting the safe Reducible one or the stdlib (which would be safe in exactly the cases that the Reducible exists).
Is that what you mean?
There was a problem hiding this comment.
@johnynek yes that's what I mean, but note that we already have syntax enrichment for Reducible, and it's generated by Simulacrum so it will automatically inherit the name of the method on the type class.
There was a problem hiding this comment.
@johnynek do you have any further thoughts on this after my note about the syntax enrichment that currently exists?
If there's a strong preference for min then I can change it, but I do have a hesitation about potential refactoring bugs that could come out of it.
There was a problem hiding this comment.
I see...
I'm not a huge implicit syntax fan (this example, but also confusion for novices as to where methods are coming from).
But I recognize many cats users do like this, so avoiding the collision seems safest.
👍
|
👍 |
|
Exemplary documentation 👍 |
This adds:
reduceLeftOptionandreduceRightOptionmethods toFoldablemaximumOptionandminimumOptionmethods toFoldablemaximumandminimummethods toReducibleResolves #1164.