Support for non-zero exit codes from CLI when there are warnings

[mrdziuban]

Prompted by my question in Discord, I wanted to open this to discuss potential approaches.

I see that there's a fatalWarnings field on the ScalafixConfig type that defaults to true. Am I misunderstanding, or should that mean that warnings should be treated as errors?

[bjaglin]

Hi @mrdziuban, thanks for reporting back here and sorry for the delay.

That fatalWarnings flag seems unused and is not exposed as a CLI arg. Judging by the git history, it controlled the reporting behavior when rules had issues introspecting the source files, but not the linter diagnostics.

I'll draft a PR to propose a new CLI or configuration flag.

[bjaglin]

Actually, I take back what I said... I just realised after exploring the test suites that there is already a pretty advanced yet undocumented CLI flag that allows to override diagnostics severity.

It looks like --settings.lint.error .* exposes almost what you are looking for: treat as errors the diagnostics raised by any rule with any category. It doesn't look like the argument can match the original severity level (warning), but maybe that is acceptable for you?

I must say that there is already so much complexity on that front that I am hesitant to add even more levers...

Implementation

https://github.com/scalacenter/scalafix/blob/a97e862d28db969eb2a84ff6ffccf004e834ad60/scalafix-cli/src/main/scala/scalafix/internal/v1/MainOps.scala#L200-L201

https://github.com/scalacenter/scalafix/blob/a97e862d28db969eb2a84ff6ffccf004e834ad60/scalafix-cli/src/main/scala/scalafix/internal/v1/DelegatingMainCallback.scala#L14-L18

https://github.com/scalacenter/scalafix/blob/a97e862d28db969eb2a84ff6ffccf004e834ad60/scalafix-core/src/main/scala/scalafix/internal/util/LintSyntax.scala#L19-L22

https://github.com/scalacenter/scalafix/blob/a97e862d28db969eb2a84ff6ffccf004e834ad60/scalafix-core/src/main/scala/scalafix/lint/LintID.scala#L16-L22

https://github.com/scalacenter/scalafix/blob/a97e862d28db969eb2a84ff6ffccf004e834ad60/scalafix-core/src/main/scala/scalafix/internal/config/LintConfig.scala#L8-L21

https://github.com/scalacenter/scalafix/blob/a97e862d28db969eb2a84ff6ffccf004e834ad60/scalafix-core/src/main/scala/scalafix/internal/config/FilterMatcher.scala#L9-L25

Example (selective to a rule/category pair)

https://github.com/scalacenter/scalafix/blob/a97e862d28db969eb2a84ff6ffccf004e834ad60/scalafix-tests/integration/src/test/scala-2/scalafix/tests/cli/CliSyntacticSuite.scala#L213-L230

https://github.com/scalacenter/scalafix/blob/a97e862d28db969eb2a84ff6ffccf004e834ad60/scalafix-tests/integration/src/main/scala/scalafix/test/cli/Rules.scala#L86-L91

Note that warning in LintWarning.warning refers to the adhoc string, not the category exposed via the LintCategory#warning() helper.

[bjaglin]

@mrdziuban any feedback on that?

[mrdziuban]

Sorry for the delay @bjaglin! I was able to get this working for my use case, which is running all syntactic rules during a pre-commit hook.

The one catch I still had to account for was making sure that any UnusedScalafixSuppression errors don't cause a non-zero exit code -- there might be unused suppressions for semantic rules that weren't run.

I was able to use --settings.lint.error like this to get it working:

--settings.lint.error '^((?!UnusedScalafixSuppression).)*$'

Ideally I could pass a flag to disable the UnusedScalafixSuppression checks completely (#1271 possibly related) but this works for now. Thanks for the help!

[bjaglin]

Thanks for the feedback. I documented our respective findings (as well as the .scalafix.conf / CLI args duality) through https://github.com/scalacenter/scalafix/pull/2062.

It's live at https://scalacenter.github.io/scalafix/docs/users/configuration.html#overriding-the-linting-behavior and below. Feel free to open a PR if that's not clear or if it does not work for you!

Ideally I could pass a flag to disable the UnusedScalafixSuppression checks completely (https://github.com/scalacenter/scalafix/issues/1271 possibly related) but this works for now

Yes let's follow up there if ever needed. The includes/excludes syntax it's a bit verbose, but at least I find it clear about the intent, so I am not sure we need more, especially now that it's documented.