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

Support several scalafixScalaBinaryVersion within a build #380

Merged
merged 11 commits into from
Dec 19, 2023

Conversation

zarthross
Copy link
Contributor

Because the scalafixInterfaceProvider was getting its scala versions from the ThisBuild level, scalafix would not use a different binary version for each scala version when running with ++scalafix. This limited users ability to use scala 2.13 only rules AND use scalafix on scala 3.

This change would make it possible to run the appropriate scalafix binary as the scala version changes during cross building (because ThisBuild / scalaVersion does not change during cross building), making it possible to dynamically update the scalafix.conf file for each scala version. In the future a sbt key could be added to point at different scalafix.conf files for different scala versions if that functionality is desired.

@armanbilge
Copy link

I don't think I fully appreciate the goals of this PR, but just based on the title, can't this already achieved something like this?

scalafixConfig := {
  if (scalaBinaryVersion.value == "3")
     Some(file(".scalafix-3.conf"))
  else
     Some(file(".scalafix-2.conf"))
}

@zarthross
Copy link
Contributor Author

So, just changing the scalafixConfig like above doesn't really solve my problem. I think the build.sbt in my scripted test was confusing, so I simplified to use the scalafixConfig setup from above. If you pull out my changes from ScalafixPlugin.scala you'll see that the scripted test fails with the following error message:

[info] [info] Setting Scala version to 2.13.12 on 1 projects.
[info] [info] Reapplying settings...
[info] [info] set current project to root (in build file:/tmp/sbt_42acdc6/)
[info] [error] (Compile / scalafix) scalafix.sbt.InvalidArgument: The ExplicitResultTypes rule needs to run with the same Scala binary version as the one used to compile target sources (2.13). To fix this problem, either remove ExplicitResultTypes from .scalafix.conf or make sure the scalafixScalaBinaryVersion setting key matches 2.13.
[info] [error] Total time: 1 s, completed Dec 2, 2023, 8:07:36 AM
[error] x sbt-scalafix/cross-build-advanced 
[error]  Cause of test exception: {line 3}  Command failed: +scalafix failed
[info] 
[error] stack trace is suppressed; run last scripted for the full output
[error] (scripted) Failed tests:
[error]         sbt-scalafix/cross-build-advanced

The reason it fails is you MUST set a scalafixScalaBinaryVersion in order to use ExplicitResultTypes, and that scalafixScalaBinaryVersion MUST be the same as the scalaBinaryVersion. When you do a cross build in sbt using ++ the ThisBuild / scalaVersion and ThisBuild / scalaBinaryVersions do not change during the build, just the project level scala versions change. So we are unable to switch the ThisBuild / scalafixScalaBinaryVersions being used by the plugin, even if we are able to change the underlying scalafixConfig.

Copy link
Collaborator

@bjaglin bjaglin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! As a follow up to the discussion above, I would suggest renaming it to "Support several scalafixScalaBinaryVersion within a build" to better carry the intent; being able to have a different configuration file to disable ExplicitResultTypes on scala3 is a related feature, which is indeed already available.

@zarthross zarthross changed the title Add initial support for different .scalafix.conf's for each scala version. Support several scalafixScalaBinaryVersion within a build Dec 3, 2023
Copy link
Collaborator

@bjaglin bjaglin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @zarthross, this will be a great addition! I'll open a PR shortly to update the docs.

For the record, I am planning to do a release some time next month, in sync with the scala 2.x patch releases. Until then, you can try the snapshots.

@@ -0,0 +1,3 @@
rules = [DisableSyntax, OrganizeImports, ExplicitResultTypes]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DisableSyntax seems unexercised, but it doesn't hurt to keep a syntactic rule here I guess

@bjaglin
Copy link
Collaborator

bjaglin commented Dec 19, 2023

1271370 had more impact than I thought (which is good to expose anyway). c964674 348e132 should get that back to green before merging.

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.

3 participants