Add Semigroup.instance method

[jozic]

No description provided.

[codecov-io]

Codecov Report

Merging #2101 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            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

Impacted Files	Coverage Δ
kernel/src/main/scala/cats/kernel/Semigroup.scala	68.75% <100%> (+4.46%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e27266a...f126e29. Read the comment docs.

[kailuowang]

Thanks.
As a quick side note, with LAMBDA SYNTAX FOR SAM TYPES in Scala 2.12, you can create a new instance even more succinctly as

def algebra[A]: Semigroup[F[A]] = self.combineK

[ceedubs]

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.

[ceedubs]

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.

[jozic]

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

[jozic]

@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

[ceedubs]

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]

@kailuowang @ceedubs
thanks for your review!
I just pushed updates to the PR as discussed

2 questions left:

1. Should I send PR with changes to catsInvariantMonoidalSemigroup.pure I excluded from this PR as they were irrelevant?
2. Should I benchmark creation of default Semigroup instances w/ and w/o usage of Semigroup.instance introduced in this PR?

[johnynek]

👍

[kailuowang]

@jozic thanks! IRT InvarianyMonoidal change, do you mind take a look at #2088?
I would say just switching back to detail creation method inside cats might be easier than benchmarking... This kind of trait instance creation is all over cats. People are so used to read it that there isn't much gain in readability in replacing them with the convenience method, IMHO.

[ceedubs]

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.

[ceedubs]

@kailuowang are you happy with this one? If so, feel free to merge.