only warn when loading rules built with newer core #1588
Conversation
The risk for incompatibility is relatively low and upgrading Scalafix is not always trivial depending on the client.
bjaglin
force-pushed
the
relax-older-version
branch
from
571b9eb
to
e3c97aa
Compare
| throw new ScalafixException( | ||
| s"Scalafix version ${Versions.nightly} cannot load the registered external rules, " + | ||
| s"please upgrade to ${dependency.getVersion} or later" | ||
| case TemptativeUp(compatibleRunWith) => |
There was a problem hiding this comment.
to clarify: this is code that will be triggered once external rules built against 0.10.1 and later are available
|
When we are going to publish |
| - Running a rule built against a more recent X.Y or 0.Y.Z version of Scalafix | ||
| is not possible. In that case, a `ScalafixException` is raised when resolving | ||
| the rules(s) artifact(s). | ||
| is also a potential source of incompatibility. In that case, a warning | ||
| calling the user to upgrade Scalafix is issued when resolving the rules(s) | ||
| artifact(s). |
There was a problem hiding this comment.
I find this case easier to deal with than using a more recent rule.
The second requires rule author to publish a new version of their rule for each major version. In the other hand, we don't publish a major version very often!
There was a problem hiding this comment.
The second requires rule author to publish a new version of their rule for each major version
With this PR and the exception turned warning, it encourages them (since they will get report from users complaining about warnings, as discussed in #1588 (comment)) but does not require them.
| args.out.println( | ||
| s"""Loading external rule(s) built against a recent version of Scalafix (${dependency.getVersion}). | ||
| |This might not be a problem, but in case you run into unexpected behavior, you | ||
| |should upgrade Scalafix to ${compatibleRunWith}. | ||
| """.stripMargin |
There was a problem hiding this comment.
The warn messages are super clear!
We can definitely turn that to an exception if many users complains that with scalafix 0.10.x, rules in 0.10.x x>0, it breaks. It would mean we don't pay too much attention to warn messages and in this case an exception is better.
There was a problem hiding this comment.
The warn messages are super clear!
Thanks, I spent a lot of time on them trying to get the right tone and making them actionnable.
We can definitely turn that to an exception if many users complains that with scalafix 0.10.x, rules in 0.10.x x>0, it breaks
What do you mean? 0.10.x runtimes will already be in the wild. we can't retroactively turn the warning into an exception.
There was a problem hiding this comment.
What I mean, we can still turn it into an exception in 0.10.5 for example, if we think a warning is not enough. I don't know if it's ok to that.
This message is only displayed when we have a tentativeUp starting 0.10.0.
The only case will be: A rule is published with 0.10.1 and the user has upgraded the rule, but not the scalafix version which is still 0.10.0.
Maybe throwing an exception is ok in that case.
There was a problem hiding this comment.
What I mean, we can still turn it into an exception in 0.10.5 for example, if we think a warning is not enough. I don't know if it's ok to that.
We can, but we would want to be more verbose in case 0.10.5 (or 0.11.0) brings silent changes important enough to prevent users to use an earlier version to run rules built/tested against it - however, it would be too late as 0.10.4 and earlier are out!
There was a problem hiding this comment.
The only case will be: A rule is published with 0.10.1 and the user has upgraded the rule, but not the scalafix version which is still 0.10.0.
Maybe throwing an exception is ok in that case, this PR changes this.
That's the current state on master/RC1.
There was a problem hiding this comment.
As discussed, let's for go for warn now. We can revisit for 1.x, maybe with an exception if build is newer by major or minor only, keeping a warn for patch diff.
Indeed, that's what I tried to explain in the release notes. I updated the message in the draft https://github.com/scalacenter/scalafix/releases/tag/untagged-7be1f5cfc653f2fefbfc to reflect the changes in this PR.
With or without Steward, it's likely that people first upgrade sbt-scalafix (and other clients, as I opened PR for them to target 0.10.0 as early as possible) since rules will lag a bit behind, so they will get a warning. The value of the warning is that it will encourage rule authors to build against 0.10.x, so that we can potentially drop support for 0.9.x-built rules in 0.11.x or 1.x. (#1546 does the job but is hacky). |
|
@tgodzik it might be hard to follow the various discussions, but your feedback would be welcome here (with both your Metals and scalameta hat). |
On the metals front, my (unchecked) assumption is that a given metals release pins both scalafix & the organize import rule, in which case versioning/compatibility is not a concern. If that's not the case (user-defined rules?), then it might be worth looking at how/if metals would handle warnings. |
|
So on metals, your assumption is correct. |
You're correct, we have both of those pinned, so if I'm following this all correctly this shouldn't impact stuff at all since the only external rule we're implemented with is the organize imports one. In the future if we ever add the functionality to run user-defined rules, then we'll cross that bridge when we get there 😆 |
Follow-up of #1565 (c4c1f1e#r69611050)