Skip to content
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

Fix broken binary compatibility #3163

Merged
merged 4 commits into from
Nov 26, 2019

Conversation

travisbrown
Copy link
Contributor

Reverts #3100, #3105, and undoes part of #2834. This unfixes #2891 and #2701, which I think will have to wait for 3.0.

In the case of #2834 I don't think the move was necessary, since the CommutativeMonoid one is more specific. In any case moving the other back doesn't break any tests and seems fine:

scala> import cats.kernel.instances.all._
import cats.kernel.instances.all._

scala> cats.kernel.CommutativeMonoid[Int]
res0: cats.kernel.CommutativeMonoid[Int] = cats.kernel.instances.IntGroup@5c7654d1

scala> cats.kernel.Monoid[Int]
res1: cats.kernel.Monoid[Int] = cats.kernel.instances.IntGroup@5c7654d1

scala> cats.kernel.Monoid[String]
res2: cats.kernel.Monoid[String] = cats.kernel.instances.StringMonoid@77c600b4

I've confirmed that my quick-fix MiMa branch here doesn't find any problems after this change and #3162.

@codecov-io
Copy link

codecov-io commented Nov 19, 2019

Codecov Report

Merging #3163 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3163   +/-   ##
=======================================
  Coverage   93.04%   93.04%           
=======================================
  Files         376      376           
  Lines        7374     7374           
  Branches      209      209           
=======================================
  Hits         6861     6861           
  Misses        513      513
Flag Coverage Δ
#scala_version_212 93.37% <100%> (+0.02%) ⬆️
#scala_version_213 90.62% <100%> (-0.02%) ⬇️
Impacted Files Coverage Δ
.../scala/cats/kernel/instances/OptionInstances.scala 100% <100%> (ø) ⬆️
...n/scala/cats/kernel/instances/QueueInstances.scala 100% <100%> (ø) ⬆️
...in/scala/cats/kernel/instances/ListInstances.scala 100% <100%> (ø) ⬆️
.../scala/cats/kernel/instances/VectorInstances.scala 100% <100%> (ø) ⬆️

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 55c48a8...d6e940b. Read the comment docs.

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a discussion of how to make sure we don't forget these otherwise good changes for an eventual 3.0, but that conversation shouldn't block 2.x. 👍

@travisbrown
Copy link
Contributor Author

@rossabaker I was thinking we'd reopen the issues that were closed once this is merged, which I think should cover it.

@rossabaker rossabaker merged commit 6697edc into typelevel:master Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants