document sbt-scalafix scalafixScalaBinaryVersion #1166
Conversation
| @@ -155,6 +155,8 @@ Great! You are all set to use Scalafix with sbt :) | |||
| | `scalafixConfig` | `SettingKey[Option[File]]` | `.scalafix.conf` file to specify which scalafix rules should run, together with their potential options. Defaults to `.scalafix.conf` in the root directory, if it exists. | |||
| | `scalafixDependencies` | `SettingKey[Seq[ModuleID]]` | Dependencies making [custom rules](#run-custom-rules) available via their simple name. Must be set in `ThisBuild`. Defaults to `Nil`. | |||
| | `scalafixResolvers` | `SettingKey[Seq[Repository]]` | Custom resolvers where `scalafixDependencies` are resolved from. Must be set in `ThisBuild`. Defaults to: Ivy2 local, Maven Central, Sonatype releases & Sonatype snapshots. | |||
| | `scalafixScalaBinaryVersion` | `SettingKey[String]` | Scala binary version used for Scalafix execution. Defaults to 2.12. For advanced rules such as ExplicitResultTypes to work, it must match the binary version defined in the build for compiling sources. Note that `scalafixDependencies` artifacts must be published against that Scala version. | |||
There was a problem hiding this comment.
Note that
scalafixDependenciesartifacts must be published against that Scala version.
That's a bit vague on purpose, as I didn't want to explain which full Scala version would be used in case we have external rules using the presentation compiler.
| s"The ExplicitResultTypes rule only supports the Scala version '$supportedScalaVersion' for this Scala " + | ||
| "binary version. To fix this problem, either remove `ExplicitResultTypes` from .scalafix.conf, or adjust " + | ||
| s"the patch Scala version in your build to match exactly '$supportedScalaVersion' after making sure that " + | ||
| s"the `scalafixBinaryScalaVersion` setting key matches the Scala binary version of your build." |
There was a problem hiding this comment.
Not very happy about that as the first sentence can be confusing when binary scala version does not match.... Ideally, we should get supportedScalaVersion from scalafix-cli based on config.scalaVersion, but that's impossible. Any suggestion is welcome!
There was a problem hiding this comment.
After re-reading scalacenter/sbt-scalafix#121 (comment), I was wondering if we could simply remove the full version constraint here and focus on the binary version (maybe by cross-testing rules at the patch level to verify compatibility)?
There was a problem hiding this comment.
I don't understand exactly what you mean, but I agree that now this rule could be used for 2.11, 2.12, and 2.13..
You would like to test many version of the same 2.13 ??
There was a problem hiding this comment.
Currently, the rule will fail verbosely if the sources were compiled with a different patch version than the one scalafix-rules was built against.
It's either:
- a different patch version, in which case the user must update its built version to match the one from
scalafix-rules - a different binary version, in which case the user must align the
scalafixScalaBinaryVersionAND potentially the patch release as in (1). The first sentence is a bit confusing as it will say "The ExplicitResultTypes rule only supports the Scala version 2.12.11" for sources compiled in 2.13.1 for example, which is not true as there is now support for 2.13.2.
That's why I was wondering if we should relax the constraint to only fail on a binary version mismatch, and assume it will work most of the time for patch version mismatch. That's my interpretation of @olafurpg's comment. In that case, it might be wise to run the testkit against input cross-built against many patch versions (and not just one per binary version), although this will significantly increase CI time if we are exhaustive for 2.11, 2.12 & 2.13.
So my comment/suggestion is not related to this PR, but writing the recommendation on failure made me realize that having 2 different cases and actions makes it hard to use.
I am not familiar with the presentation compiler and how it's used in the rule to judge whether it's acceptable to ignore the patch release even for built-in rules, but since scalafix-cli is cross-built with a full version, I guess there was a reason?
There was a problem hiding this comment.
In my opinion, we should relax this constraint.
I propose to support 3 scala versions of the compiler for each binary version of Scala, by the corresponding CI-tests.
It will increase ci-tests, but I think it's acceptable.
There was a problem hiding this comment.
Just randomly jumping in here 😬, but quick question on the wording here. I actually just came across this while using https://github.com/joan38/mill-scalafix. I hit on this issue the message produced seems to be very sbt-specific while there is no guarantee that the user is using sbt here. I'm just curious if there is a better way to word this and avoid the setting key. I'm all for making error messages clear and actionable, but when the build tool is assumed, then it can get really confusing for someone not familiar with the ecosystem.
|
What do you think about merging this PR and I will finish/rebase this PR on top of it ? |
|
I'll rebase this against #1174 once it's merged |
bjaglin
force-pushed
the
scalafixScalaBinaryVersion
branch
from
c3a9eba to
a085542
Compare
|
@mlachkar that's rebased! |
Merge only when scalacenter/sbt-scalafix#121 is released
Improves #998