Add a CommutativeMonoid for Map

[ceedubs]

Since we already have a Monoid instance for Map, it seems natural to
have a CommutativeMonoid when the value type has a commutative
semigroup.

Note: Unfortunately, I believe that this is a binary-incompatible change in cats kernel. cc @johnynek

[codecov-io]

Codecov Report

Merging #2098 into master will increase coverage by 0.12%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2098      +/-   ##
==========================================
+ Coverage   94.65%   94.78%   +0.12%     
==========================================
  Files         322      325       +3     
  Lines        5449     5488      +39     
  Branches      215      212       -3     
==========================================
+ Hits         5158     5202      +44     
+ Misses        291      286       -5

Impacted Files	Coverage Δ
...nel/src/main/scala/cats/kernel/instances/map.scala	100% <100%> (ø)	⬆️
core/src/main/scala/cats/instances/function.scala	93.75% <0%> (ø)	⬆️
...ws/src/main/scala/cats/laws/DistributiveLaws.scala	100% <0%> (ø)	⬆️
core/src/main/scala/cats/arrow/Choice.scala	50% <0%> (ø)	⬆️
...scala/cats/laws/discipline/DistributiveTests.scala	100% <0%> (ø)	⬆️
core/src/main/scala/cats/instances/tuple.scala	100% <0%> (ø)	⬆️
core/src/main/scala/cats/arrow/ArrowChoice.scala	100% <0%> (ø)
...aws/src/main/scala/cats/laws/ArrowChoiceLaws.scala	100% <0%> (ø)
.../scala/cats/laws/discipline/ArrowChoiceTests.scala	100% <0%> (ø)
core/src/main/scala/cats/data/Kleisli.scala	97.84% <0%> (+0.07%)	⬆️
... and 2 more

---

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 b91e9bb...fa99829. Read the comment docs.

[johnynek]

Algebird is not using the instances object, so this won't burn algebird. @denisrosset can confirm if this will hurt spire.

[LukaJCB]

LGTM, will merge if we can confirm this won't affect Spire :)

[denisrosset]

Right now, Spire has its own instances; the proposed definition is sane and sound.

I have a minor complaint: both the commutative and noncommutative tests of Monoid[Map[K,V]] use commutative monoids on the values.

Consider changing the Monoid[Map[String, Int]] to Monoid[Map[String, String]], so that if the instance does not behave correctly with noncommutative types, the tests can catch it. This problem is currently biting us in Spire.

[ceedubs]

@denisrosset thanks for the review. You have a good point regarding the tests. Just to confirm that I understand you, you are suggesting leaving the new test as it is, but changing the existing MonoidTests[Map[String, Int]] call to MonoidTests[Map[String, String]], right?

[denisrosset]

@ceedubs, exactly. Not a big deal in this case, but a good habit to keep: try to not use for tests an instance that has more structure (i.e. commutativity) than required.

[ceedubs]

@denisrosset 👍 that sounds like a good general guideline.

Oh thanks @LukaJCB you beat me to it.

[johnynek]

a non commutative semigroup that does not accumulate in memory (like String) does is:
(a0, a1) + (b0, b1) = (a0, b1)

This can be for any (A, B).

[ceedubs]

@johnynek on that note, wouldn't a simplified version be the "always first" semigroup? a0 |+| a1 = a0?

[johnynek]

Indeed it would.