Add Semigroup.instance method #2101
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2101 +/- ##
==========================================
+ Coverage 94.75% 94.75% +<.01%
==========================================
Files 322 322
Lines 5454 5456 +2
Branches 214 224 +10
==========================================
+ Hits 5168 5170 +2
Misses 286 286
Continue to review full report at Codecov.
|
ceedubs
left a comment
ceedubs
left a comment
There was a problem hiding this comment.
Thanks, @jozic!
I do think that the instance method is convenient if there are people stuck on Java versions in which they can't use the SAM syntax that @kailuowang mentioned.
I don't know whether it would be significant enough to worry about, but I do wonder about slight performance implications of the Semigroup definitions for Either, Validated, etc using Semigroup.instance as opposed to what they were using before.
Please see my comment about the unrelated change in the pure implementation for Semigroup.
| if (as.isEmpty) None | ||
| else if (as.size == 1) as.toList.headOption | ||
| else Some(a) | ||
| if (as.size > 1) Some(a) else as.toTraversable.headOption |
There was a problem hiding this comment.
There may be merit to these changes, but they seem to be unrelated to the rest of the PR, and I think that they should be separated out into another PR. I'm not sure if there are types that implement TraversableOnce for which an isEmpty check would be significantly faster than a size check on an empty collection, but it's something that people might not think about when reviewing this PR.
There was a problem hiding this comment.
Np, I will revert these changes, they are indeed not related to Semigroup.instance
should I open a new PR for it?
my first assumption here was that toTraversable convertion instead of toList can be cheaper/faster
second one is that headOption already does same check as removed line 22
There was a problem hiding this comment.
@kailuowang that's true, but unfortunately not all of us are in Java 8/Scala 2.12 lands yet
@ceedubs
I can write jmh benchmarks for both cases - the old way of creating instances and the new one using Semigroup.instance
OR
alternatively I can revert those changes and just leave addition of Semigroup.instance , as this is the main goal here. Then we can try another PR with benchmarks or just let it go
Please let me know
There was a problem hiding this comment.
I can revert those changes and just leave addition of Semigroup.instance , as this is the main goal here. Then we can try another PR with benchmarks or just let it go
@jozic this sounds like the path of least resistance to me. Just please be sure to add a test for the Semigroup.instance method, since I think that currently it's only indirectly tested through the Semigroup instances that you are using it to create.
jozic
force-pushed
the
semigroup-instance
branch
from
4c83899 to
f126e29
Compare
|
@kailuowang @ceedubs 2 questions left:
|
|
👍 |
|
@jozic thanks! IRT InvarianyMonoidal change, do you mind take a look at #2088? |
ceedubs
left a comment
ceedubs
left a comment
There was a problem hiding this comment.
Nice. Thanks again, @jozic!
I agree with @kailuowang's comment on the questions that you asked. I don't personally see much benefit in the change to the pure implementation, but I don't have strong feelings on it.
|
@kailuowang are you happy with this one? If so, feel free to merge. |
5 participants
No description provided.