Open armanbilge opened 2 years ago
Thanks for the report!
However, in this case, rules was not missing, merely empty :)
Indeed, I guess a better wording could be "no rule requested to run" (since rules can come from CLI args or the configuration file).
I'd like to hear more about your use-case to understand why you think this should not be an error (possibly just a warning?). Are you running scalafix through sbt-scalafix or another build tool, targeting several modules with different .scalafix.conf
maybe?
Thanks for the response!
I'd like to hear more about your use-case to understand why you think this should not be an error (possibly just a warning?). Are you running scalafix through sbt-scalafix or another build tool
Yes, we're rolling out sbt-scalafix en-masse to an entire organization. https://github.com/http4s/sbt-http4s-org/releases/tag/v0.14.0
There's a lot of automation in there, including auto-generated GitHub actions workflows which enforce scalafix checks in CI.
Ideally, there would be a convenient way for individual maintainers to disable any scalafix rules for their repo.
For example, we also roll out scalafmt in the same way, but it's easy to opt out with this config:
project.includePaths = [] # disables formatting
targeting several modules with different
.scalafix.conf
maybe?
Sure, this would be convenient for that too, if you don't want any rules for Test
config for example
For the context: for sbt-scalafix users, running scalafix
without any configuration-provided nor CLI-provided rule means that the invocation is a no-op, which is most likely a setup issue, thus the verbose failure. I am hesitant to change that behavior, and I am trying to challenge the introduction of another flag that could change that behavior.
https://github.com/http4s/sbt-http4s-org/releases/tag/v0.14.0
Just a side note about the release notes: they mention triggered.rules
, but is scalafixOnCompile
enabled somehwere?
Ideally, there would be a convenient way for individual maintainers to disable any scalafix rules for their repo.
I am assuming that "maintainers" here refer to "maintainers using sbt-typelevel", right? It looks like tlCiScalafix
does avoid CI invocations at least - in which other context would you like to "disable scalafix"?
For example, we also roll out scalafmt in the same way, but it's easy to opt out with this config:
From what I can see, TypelevelScalafixPlugin
triggers automatically and adds semanticdb, so just adding the sbt-typelevel plugin has a side effect on compilation, no matter if we have a "no-op" option in scalafix like the one you describe for scalafmt.
I guess it goes against the design of sbt-typelevel, but maybe this plugin should be added explicitly by maintainers? In that case, you could define typelevelScalafix
/typelevelScalafixCheck
task keys (wrapping the scalafix
input task key), which would thus only aggregate on projects where the project is explicitly setup (and where we expect a non-empty set of rules defined).
Sure, this would be convenient for that too, if you don't want any rules for Test config for example
Actually, that's already possible, as the scalafixConfig
key is looked up at the configuration level. I'll add a scripted to guard that useful behavior (https://github.com/scalacenter/sbt-scalafix/pull/305).
I was asking because that's a use-case I can see with sbt-scalafix: calling scalafixAll
should probably not fail if we have rules set up for IntegrationTest
and Compile
but not for Test
(based on different configuration files). I need to think about that.
Thank you so much for the detailed response!
For the context: for sbt-scalafix users, running
scalafix
without any configuration-provided nor CLI-provided rule means that the invocation is a no-op, which is most likely a setup issue, thus the verbose failure.
For the record, I 100% agree with you. This should definitely be a verbose failure.
By opening this issue I was hoping you might distinguish between:
# empty file, no config
and:
rules = [] # I *deliberately* set rules to empty
In the latter case, configuration has been provided.
Actually, that's already possible, as the
scalafixConfig
key is looked up at the configuration level. I'll add a scripted to guard that useful behavior (scalacenter/sbt-scalafix#305).
Yes, thanks for the PR! I am indeed aware it is possible to have separate scalafix configs for Compile
, Test
, etc. But my question is, what if I don't want any rules enabled for Test
, only for Compile
. How can I configure that?
Here's an example where a project wanted different scalafix configurations for different projects. For some legacy modules, they wanted to enable no rules, to avoid code churn.
In the end, it was necessary to still specify one rule as a placeholder.
Technically speaking, we could try wrapping https://github.com/scalacenter/scalafix/blob/57042bd97ef97e832c2701eecc6770672b980492/scalafix-cli/src/main/scala/scalafix/internal/v1/Args.scala#L54 in an Option
to see whether we can have a no-op, non-failing behavior with an explicit empty list of rules.
I do feel like this would be fixing the problem at the wrong level though since we don't want to be calling scalafix to figure out it has nothing to do but rather not call scalafix at all. What about honoring scalafix / skip
at the configuration level? You could also look at this flag to conditionally enable semanticdb in TypelevelScalafixPlugin
, so that a skipped project/configuration really doesn't add overhead.
Somewhat related: https://github.com/scalacenter/scalafix/issues/1413.
What about honoring
scalafix / skip
at the configuration level? You could also look at this flag to conditionally enable semanticdb inTypelevelScalafixPlugin
, so that a skipped project/configuration really doesn't add overhead.
So here's an interesting counterpoint: I asked for sbt-mima to support skip
in https://github.com/lightbend/mima/issues/684, and instead was told to use mimaPreviousArtifacts := Set.empty
😂
Of course, that shouldn't dictate what is the right thing to do here. My larger point is that unfortunately skip
support across the sbt ecosystem is very unreliable, and as much as I like the concept I've been burned in the past e.g. https://github.com/typelevel/sbt-typelevel/issues/140. Reading https://github.com/scalacenter/scalafix/issues/1413 it seems you may have encountered similar issues.
I do feel like this would be fixing the problem at the wrong level though since we don't want to be calling scalafix to figure out it has nothing to do but rather not call scalafix at all.
I'm not sure if it's the wrong level. It depends on whether you consider this an issue specific to sbt-scalafix or scalafix in general, in which case the .scalafix.conf
seems the right place to address this.
Although less common there are ways to run scalafix outside of sbt. And maybe in the age of scala-cli-based projects these non-sbt ways of doing things will become more important. Also metals does things outside of sbt.
For example, in orgs it's often useful to distribute a common configuration* e.g. .scalafix-common.conf
that CI enforces stays up-to-date against a master copy. Then, individual projects may override settings on top of it:
# .scalafix.conf
include ".scalafix-common.conf"
rules += "AnotherRule"
This is another situation where for some reason, perhaps a project wants/needs to explicitly disable all rules.
* FTR is the setup recommended by scalafmt for sharing configuration: https://scalameta.org/scalafmt/docs/installation.html#share-configuration-between-builds
I'm not sure if it's the wrong level. It depends on whether you consider this an issue specific to sbt-scalafix or scalafix in general, in which case the .scalafix.conf seems the right place to address this.
Although less common there are ways to run scalafix outside of sbt. And maybe in the age of scala-cli-based projects these non-sbt ways of doing things will become more important. Also metals does things outside of sbt.
My point was that any semantic-rule-ready client must enable semanticdb upfront before calling scalafix, since scalafix expects semanticdb files to be available. So I was advocating to shift left in order to prevent an unnecessary increase of build time when scalafix is going to end up doing nothing. I am just still unsure how. We could expose whether scalafix is going to run or not as part of the API, but it might be too late for some clients, like sbt-scalafix.
For the record, the code handling rules = []
is there: https://github.com/scalacenter/scalafix/blob/57042bd97ef97e832c2701eecc6770672b980492/scalafix-cli/src/main/scala/scalafix/internal/v1/Args.scala#L273
If I have
.scalafix.conf
:then scalafix runs fine.
However if I have
.scalafix.conf
:Then I get:
However, in this case,
rules
was not missing, merely empty :)