-
-
Notifications
You must be signed in to change notification settings - Fork 384
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
Twirllib: new twirlScalaVersion target to support for newer versions #1959
Conversation
james-s-w-clark
commented
Jul 18, 2022
- https://github.com/playframework/twirl/releases 1.5.1 is latest
- updated docs to latest version
I need to update my fork with changes from main. Will do soon 🥵 |
@@ -25,10 +25,10 @@ trait TwirlModule extends mill.Module { | |||
coursier.LocalRepositories.ivy2Local, | |||
MavenRepository("https://repo1.maven.org/maven2") | |||
), | |||
Lib.depToDependency(_, "2.12.5"), | |||
Lib.depToDependency(_, "2.13.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 the twirl compiler now on Scala 2.13?
And even if so, we may want to keep support of older versions, at least in Mill 0.10.x
Alternatively, we may state some (non-)compatibility notes in the plugin documentation page and the release notes.
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.
2.13 support since twirl 1.4.2 https://mvnrepository.com/artifact/com.typesafe.play/twirl-compiler
Does this line of code mean it's either 2.12 or 2.13 support? I don't know much about the whole "binary incompatibility" situation. Is it a lot more complex to support older versions too?
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.
In this specific use case, it means, all Scala ivy deps (those with a double colon before the name) will resolve to the given Scala version. You can't mix them in the same classpath, so yes, it's either 2.12 or 2.13.
We can support both versions and we should, but it may not work with this hardcoded version here.
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 think we should make sure, that we have tests for older and never twirl versions with Scala 2.13 and 2.12. I've no idea how widely this is used, though.
That makes sense. I had never heard of twirl before looking into this. |
The new `twirlScalaVersion` target is mandatory for now. Also added more test to test with Scala 2.12 and 2.13
@lefou thank you for finishing this one off :) |