-
Notifications
You must be signed in to change notification settings - Fork 185
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
Migrate to 2.13 #1118
Migrate to 2.13 #1118
Conversation
project/Dependencies.scala
Outdated
@@ -21,7 +21,8 @@ object Dependencies { | |||
def metacp = "org.scalameta" %% "metacp" % scalametaV | |||
def semanticdbPluginLibrary = "org.scalameta" % "semanticdb-scalac-core" % scalametaV cross CrossVersion.full | |||
def scalameta = "org.scalameta" %% "scalameta" % scalametaV | |||
def scalatest = "org.scalatest" %% "scalatest" % "3.0.8" | |||
def bijectionCore = "com.twitter" %% "bijection-core" % "0.9.7" | |||
def scalatest = "org.scalatest" %% "scalatest" % "3.1.2" |
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 a reason this upgrade is needed? The upgrade was reverted in #1083
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 is no real reason for that. I was just updating some libs, removing some todo, and adding the real change on top of that.
I will remove that.
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 looks great! I'm excited to finally cross-build against 2.13. Just a few non-critical comments. Feel free to merge
One thought, we may want to replace usage of scala.Seq
with scala.collection.Seq
in all places of the public API to smoothen the migration for 3rdparty rules. For example, in the method below we previously accepted mutable collections in PatchOps.replaceSymbol()
but in 2.13 it would only accept immutable collections. I think it would be nice to continue to allow mutable collections there, but I don't feel strongly about it
toReplace: Seq[(String, String)] |
private val results: List[Int] = List.empty | ||
protected val results2: List[Int] = List.empty | ||
} | ||
//todo: in 2.13, the type found is AnyRef |
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's usually better to open an issue in my experience and link to from here instead of adding a todo 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.
I will create all corresponding issues.
scalafix-tests/unit/src/test/scala/scalafix/tests/rule/MavenFuzzSuite.scala
Show resolved
Hide resolved
I think this line if (isScala213.value) "-Ywarn-unused:imports" should be |
You're right @ohze. Thanks for noticing that. |
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.
LGTM 👍 Thank you @mlachkar for driving this upgrade!
* update bijection-core 0.9.7 * Migrate to 2.13 * remove fixed todo * Fix warnings in 2.13 and keep scala.collection.Seq for the api * Fix scalaOption for unused import in 2.13
The missing points that need to be addressed to complete this migration to 2.13: