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

add back the implicit method as explicit to maintain BC #2471

Merged
merged 3 commits into from
Sep 5, 2018

Conversation

kailuowang
Copy link
Contributor

No description provided.

LukaJCB
LukaJCB previously approved these changes Sep 5, 2018
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.

Looks good, but I'm going to be super pedantic and go through the rest just so we don't need a 1.3.2 for this. Stay tuned.

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.

I tested all the other MiMa exceptions, and those are sound.

We can also remove this exception, since it shouldn't have been:

exclude[DirectMissingMethodProblem]("cats.data.OptionTInstances.catsDataMonadForOptionT")

rossabaker
rossabaker previously approved these changes Sep 5, 2018
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.

Yep, perfect.

@kailuowang
Copy link
Contributor Author

@rossabaker I just removed the exception

@kailuowang kailuowang added the bug label Sep 5, 2018
ceedubs
ceedubs previously approved these changes Sep 5, 2018
LukaJCB
LukaJCB previously approved these changes Sep 5, 2018
@kailuowang kailuowang added this to the 1.3.1 milestone Sep 5, 2018
@LukaJCB LukaJCB dismissed stale reviews from ceedubs, rossabaker, and themself via 0a32f44 September 5, 2018 16:20
Copy link
Contributor

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

It’s dangerous to add to the Mima exceptions I guess.

@rossabaker
Copy link
Member

rossabaker commented Sep 5, 2018

We could have automated test on exceptions. Compile this in an unpublished cats module that builds against the desired cats-core in Provided scope:

object MimaExceptions {
    def isBinaryCompatible = {
      cats.data.OptionT.catsDataTraverseForOptionT[List]
      cats.data.Kleisli.catsDataCommutativeArrowForKleisliId
      cats.data.OptionT.catsDataMonoidKForOptionT[List]
      cats.data.OptionT.catsDataMonoidForOptionT[List, Int]
      cats.data.Kleisli.catsDataMonadForKleisliId[Int]
      cats.data.Kleisli.catsDataCommutativeArrowForKleisli[Option]
      cats.data.Kleisli.catsDataCommutativeFlatMapForKleisli[Option, Int]
      cats.data.IRWST.catsDataStrongForIRWST[List, Int, Int, Int]
      cats.data.OptionT.catsDataMonadErrorMonadForOptionT[List]
      true
  }
}

And then the test, which depends on the unpublished module:

class MimaExceptionsTest {
  test("is binary compatible") { 
    MimaExceptions.isBinaryCompatible
  }
}

It's more or less what I did in a side repo to verify the others. And it would have caught this one.

@rossabaker
Copy link
Member

Note: I don't want the above test idea to slow the bugfix. If others like the idea, we can always add it after 1.3.1.

@codecov-io
Copy link

Codecov Report

Merging #2471 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2471      +/-   ##
==========================================
- Coverage   95.39%   95.38%   -0.02%     
==========================================
  Files         357      357              
  Lines        6516     6517       +1     
  Branches      288      282       -6     
==========================================
  Hits         6216     6216              
- Misses        300      301       +1
Impacted Files Coverage Δ
core/src/main/scala/cats/data/OptionT.scala 95.91% <0%> (-0.99%) ⬇️

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 b3c704f...0a32f44. Read the comment docs.

@LukaJCB LukaJCB merged commit 7f6309d into master Sep 5, 2018
kailuowang added a commit that referenced this pull request Sep 5, 2018
* add back the implicit method as explicit to maintain BC

* Update build.sbt

* Please scalastyle
@kailuowang kailuowang deleted the kailuowang-patch-4 branch September 6, 2018 07:35
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.

6 participants