-
-
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
Semigroup object additions #4625
base: main
Are you sure you want to change the base?
Conversation
It seems this PR was overlooked... Where do we stand on this? As I can see, there are some tests for the former static methods in |
I guess adding it costs little, although the style this represents (which we used in Algebird at twitter to simplify type inference) isn't used outside of algebra (which adopted it from algebird) That said, since those files are using it, I can see the argument for being consistent and I think we are unlikely to add more methods to the traits needed many more updates. I would be for it to improve consistency with existing code. |
@@ -178,6 +178,11 @@ object Semigroup | |||
*/ | |||
@inline def last[A]: Semigroup[A] = (_, y) => y | |||
|
|||
@inline def reverse[A](implicit ev: Semigroup[A]): Semigroup[A] = ev.reverse |
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 don't see a huge benefit here since to use it you will often need Semigroup.reverse[String]
compared to Semigroup[String].reverse
which isn't a huge difference. You could avoid the type by passing to a method that expects a certain type, which may be common (since you are passing to an implicit/using in many cases).
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. The reason I had added it was for consistency with the other methods.
In any case, I've removed the commit adding reverse
and kept just the one adding intercalate
.
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.
We could say the rule is: if the method takes a parameter of that is sufficient to infer the type of the result we add this method. In that way, it's still consistent.
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.
True. I think that's a pretty good rule. Thanks for your review.
99714f1
to
5db6c0f
Compare
5db6c0f
to
291ef19
Compare
Adresses: #4615