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 support for 2.13.0-M3 #2203

Merged
merged 5 commits into from Apr 2, 2018
Merged

Add support for 2.13.0-M3 #2203

merged 5 commits into from Apr 2, 2018

Conversation

ghost
Copy link

@ghost ghost commented Mar 19, 2018

@ghost
Copy link
Author

ghost commented Mar 19, 2018

@tpolecat, @eperinan For the build to work, I had to explicitly add the tut plugin, otherwise it was trying to find the 0.6.2 version, so something is not quite right here

see https://github.com/typelevel/cats/pull/2203/files#diff-a84f91f20f040218bccd09fed4761fb3R16

@codecov-io
Copy link

codecov-io commented Mar 20, 2018

Codecov Report

Merging #2203 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

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

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

@ghost
Copy link
Author

ghost commented Mar 20, 2018

@tpolecat, @eperinan , here's a printout :

[warn] Found version conflict(s) in library dependencies; some are suspected to be binary incompatible:
[warn] * org.tpolecat:tut-plugin:0.6.3 is selected over 0.6.2
[warn] +- default:catalysts-bh-topic-sbt-catalaysts-0-8-5-build:0.1.0-SNAPSHOT (scalaVersion=2.12, sbtVersion=1.0) (depends on 0.6.3)
[warn] +- com.47deg:sbt-microsites:0.7.17 (scalaVersion=2.12, sbtVersion=1.0) (depends on 0.6.2)

kailuowang
kailuowang previously approved these changes Mar 20, 2018
@ghost
Copy link
Author

ghost commented Mar 24, 2018

Rebased on master, as that was blocked

@ceedubs
Copy link
Contributor

ceedubs commented Mar 24, 2018

@BennyHill why is it that this also includes 1.1 release notes, author updates, etc?

@ghost
Copy link
Author

ghost commented Mar 24, 2018

@BennyHill why is it that this also includes 1.1 release notes, author updates, etc?

I rebased on master. see before - merge was blocked

@ceedubs
Copy link
Contributor

ceedubs commented Mar 24, 2018

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.

@ghost
Copy link
Author

ghost commented Mar 24, 2018

Ok, I'll do a squash in the morning to fix them up

@ghost
Copy link
Author

ghost commented Mar 24, 2018

Hmm, looks the same in #2200 ?

@ghost
Copy link
Author

ghost commented Mar 25, 2018

Done, ready for review

Copy link
Contributor

@ceedubs ceedubs left a 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
Copy link
Contributor

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?

Copy link
Author

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?

Copy link
Author

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)

Copy link
Contributor

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:
Copy link
Contributor

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.

Copy link
Author

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"))
Copy link
Contributor

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

Copy link
Author

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 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you 👍

Copy link
Contributor

@ceedubs ceedubs left a 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!

Copy link
Member

@LukaJCB LukaJCB left a 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!

@kailuowang kailuowang merged commit 2ae785f into typelevel:master Apr 2, 2018
@kailuowang kailuowang mentioned this pull request Apr 30, 2018
@kailuowang kailuowang added this to the 1.2 milestone May 30, 2018
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