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

Twirllib: new twirlScalaVersion target to support for newer versions #1959

Merged
merged 7 commits into from
Jul 25, 2022

Conversation

james-s-w-clark
Copy link
Contributor

@james-s-w-clark
Copy link
Contributor Author

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

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Member

@lefou lefou left a 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.

@james-s-w-clark
Copy link
Contributor Author

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.
Like with the proguard updates PR, maybe it's best to leave these updates to people who need/want them - I have to direct energy elsewhere for a while.

@lefou lefou changed the title Update twirllib Twirllib: new twirlScalaVersion target to support for newer versions Jul 25, 2022
@lefou lefou merged commit bd05a12 into com-lihaoyi:main Jul 25, 2022
@lefou lefou added this to the after 0.10.5 milestone Jul 25, 2022
@james-s-w-clark
Copy link
Contributor Author

@lefou thank you for finishing this one off :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants