-
Notifications
You must be signed in to change notification settings - Fork 75
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
Adds crossover for Scala 2.10 and upgrades libraries #46
Adds crossover for Scala 2.10 and upgrades libraries #46
Conversation
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 agree with bumping the version as you suggested @fedefernandez. Other than that, LGTM!!
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.
BTW, Travis should compile and run tests taking into account the scala version:
https://github.com/47deg/github4s/blob/master/.travis.yml
(sbt ++$TRAVIS_SCALA_VERSION ...
)
@@ -19,6 +19,11 @@ lazy val buildSettings = Seq( | |||
homepage := Option(url("http://47deg.github.io/github4s/")), | |||
organizationHomepage := Option(new URL("http://47deg.com")), | |||
scalaVersion := "2.11.8", | |||
crossScalaVersions := Seq("2.10.6", scalaVersion.value), |
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.
Could you please solve this issue by the way?
#40
crossScalaVersions := Seq("2.10.6", "2.11.8", "2.12.1")
would be great ;) . Thanks!
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.
There are some incompatibilities with the RosHTTP
library (see this build). I'm going to send a PR for adding support to Scala 2.12 in RosHTTP
but in the meantime, I think we should go forward with this. What do you think?
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.
👍
Current coverage is 88.00% (diff: 98.33%)@@ master #46 diff @@
==========================================
Files 21 22 +1
Lines 306 350 +44
Methods 304 348 +44
Messages 0 0
Branches 2 2
==========================================
+ Hits 265 308 +43
- Misses 41 42 +1
Partials 0 0
|
@@ -19,6 +19,11 @@ lazy val buildSettings = Seq( | |||
homepage := Option(url("http://47deg.github.io/github4s/")), | |||
organizationHomepage := Option(new URL("http://47deg.com")), | |||
scalaVersion := "2.11.8", | |||
crossScalaVersions := Seq("2.10.6", scalaVersion.value), |
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 PR:
case class
ReposUrls
had more than 22 fields, I've replaced all optional fields for a map, adding compatibility with scala 2.10sbt-catalysts-extras
plugin. This implies some changes in the cats library code, like replacingXor
byEither
and avoid some deleted implicits (RecursiveTailRecM
)Please @juanpedromoreno or @jdesiloniz could you review? Thanks
NOTE Should I change the version to
0.10.0-SNAPSHOT
? I think we are breaking the compatibility with the 0.9.x version changing theReposUrls
case class.