-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 support for 2.13.0-M3 #2203
Conversation
Notes on build failures:
|
@tpolecat, @eperinan For the build to work, I had to explicitly add the see https://github.com/typelevel/cats/pull/2203/files#diff-a84f91f20f040218bccd09fed4761fb3R16 |
Codecov Report
@@ Coverage Diff @@
## master #2203 +/- ##
=======================================
Coverage 95.05% 95.05%
=======================================
Files 333 333
Lines 5789 5789
Branches 211 211
=======================================
Hits 5503 5503
Misses 286 286 Continue to review full report at Codecov.
|
@tpolecat, @eperinan , here's a printout :
|
Rebased on master, as that was blocked |
@BennyHill why is it that this also includes 1.1 release notes, author updates, etc? |
I rebased on master. see before - merge was blocked |
Hmm if the changes were just brought in from master then they shouldn't be showing up in the GitHub file diff though, should they? I'm a little confused as to what's going on here. |
Ok, I'll do a squash in the morning to fix them up |
Hmm, looks the same in #2200 ? |
Done, ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a bunch, @BennyHill! I left a couple of minor comments, but this generally looks good to me.
build.sbt
Outdated
case Some((2, 13)) => | ||
"org.scalatest" %%% "scalatest" % scalatest_2_13 | ||
case _ => | ||
"org.scalatest" %%% "scalatest" % scalaTestVersion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is duplicated in two places. Should we create a def scalatestVersion(scalaVersion: String): String
or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are not quite the same, the first has a the test
scope. A "real" function looks more like this or as in catalysts. So I opted for a comment instead i,e // 2.13.0-M3 workaround
So as this is commented as an interim workaround, is this OK for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can also add a link to the comment describing this interim workaround - scalatest/scalatest#1321 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BennyHill I opened a PR against your repo with the workaround that I was thinking might clean things up a little bit. Feel free to merge if you like it: https://github.com/BennyHill/cats/pull/1
.travis.yml
Outdated
- 2.13.0-M3 | ||
|
||
matrix: | ||
allow_failures: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be helpful to leave a comment here about why this is happening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no problem, I'll add a comment
build.sbt
Outdated
@@ -401,7 +420,7 @@ lazy val bench = project.dependsOn(macrosJVM, coreJVM, freeJVM, lawsJVM) | |||
.settings(commonJvmSettings) | |||
.settings(coverageEnabled := false) | |||
.settings(libraryDependencies ++= Seq( | |||
"org.scalaz" %% "scalaz-core" % "7.2.15")) | |||
"org.scalaz" %% "scalaz-core" % "7.2.19")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7.2.20
could be used here. Very minor though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, small point but the small points tend to add up over time. Besides, might as well add this whilst doing other changes - thx for catching this 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @BennyHill!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the effort put into this!
But allow to fail on 2.13.0-M3 see