-
Notifications
You must be signed in to change notification settings - Fork 106
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
Update to Scala 2.13 #67
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.
Thank you for taking the time to open this PR, @Fristi! The work is much appreciated :)
I did not get to review this very thoroughly due to the amount of formatting changes. Please take a look at my comments below and I'll review it again afterwards.
@@ -3,78 +3,97 @@ import scalariform.formatter.preferences._ | |||
|
|||
organization in ThisBuild := "net.ruippeixotog" | |||
|
|||
scalaVersion in ThisBuild := "2.12.8" | |||
crossScalaVersions in ThisBuild := Seq("2.11.12", "2.12.8") |
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.
Is there any reason why you removed Scala 2.11? Did any library stop supporting it?
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.
Http4s does not seem to support it anymore
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.
Roughly 30% of scala-scraper downloads are for Scala 2.11, according to Sonatype. Dropping support for that Scala version just because a library that we only use in tests doesn't support it anymore doesn't seem a valid reason, unfortunately 😞
This can be addressed by migrating scala-scraper tests to a different HTTP library. It's outside the scope of this PR, however. I'll try to do it next week in order to unblock this as soon as possible.
You could consider adding this project to scala-steward to automically keep dependencies up to date |
Seems tut has trouble with comments and Scala 2.13 tpolecat/tut#246. A workaround is to only run tut for Scala 2.12. |
@@ -3,78 +3,97 @@ import scalariform.formatter.preferences._ | |||
|
|||
organization in ThisBuild := "net.ruippeixotog" | |||
|
|||
scalaVersion in ThisBuild := "2.12.8" | |||
crossScalaVersions in ThisBuild := Seq("2.11.12", "2.12.8") |
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.
Roughly 30% of scala-scraper downloads are for Scala 2.11, according to Sonatype. Dropping support for that Scala version just because a library that we only use in tests doesn't support it anymore doesn't seem a valid reason, unfortunately 😞
This can be addressed by migrating scala-scraper tests to a different HTTP library. It's outside the scope of this PR, however. I'll try to do it next week in order to unblock this as soon as possible.
Fixed the comments, let me know if everything is alright now. Feel free to squash the comments on merge :-) |
@Fristi I have just pushed the migration to akka-http for tests, as described here. We should now be able to do this without dropping support for Scala 2.11 :) Would you mind rebasing or merging this on master again? If you don't have the time I can also do it for you. Thank you for waiting for this! |
just to check if there are plans to continue this PR? I'm affected by it too (not a blocker yet.. as I have it published locally for now) |
@Fristi have you had the chance to take a look at this? I'll do the rebase myself in a few days if you don't have the availability to work on it now, in order to speed up a Scala 2.13 compatible version. Hope you don't mind :) |
Did not look at this yet, but if you have the chance go a head :) |
Resolves #66