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

Cannot disable OptionalWhenBraces with warningsAsErrors: true #6336

Closed
TWiStErRob opened this issue Jul 30, 2023 · 9 comments · Fixed by #6413
Closed

Cannot disable OptionalWhenBraces with warningsAsErrors: true #6336

TWiStErRob opened this issue Jul 30, 2023 · 9 comments · Fixed by #6413

Comments

@TWiStErRob
Copy link
Member

TWiStErRob commented Jul 30, 2023

Expected Behavior

Should be able to disable deprecated rules, or maybe be disabled by default?

Observed Behavior

Either have many OptionalWhenBraces violations or

Execution failed for task ':...:detekt'.
> Run failed with 1 invalid config property.
        - Property 'style>OptionalWhenBraces' is deprecated. Same functionality is implemented in BracesOnWhenStatements.

Steps to Reproduce

detekt {
    allRules = true
}
config:
  warningsAsErrors: true
style:
  OptionalWhenBraces:
    active: false

Context

I shouldn't have to choose between having a strict build, or having a short configuration file (allRules + overrides), or having a customized detekt config (active: false). I'm not fully sure what's going on here, and why this never happened before, but it's a strange situation and I wonder what you guys think about it.

Can you think of a workaround for this apart from disabling warningsAsErrors, or listing all the rules explicitly rather than excluding the ones I don't want?

Your Environment

  • Version of detekt used: 1.23.1
  • Version of Gradle used (if applicable): 8.3 rc 2
  • Gradle scan link (add --scan option when running the gradle task): N/A
  • Operating System and version: Windows 10
  • Link to your project (if it's a public repository): N/A
@TWiStErRob TWiStErRob added the bug label Jul 30, 2023
@mustafaozhan
Copy link

Up! With the new version 1.23.1 I start getting OptionalWhenBraces errors and when I want to disable the rule I get the exact same Property 'style>OptionalWhenBraces' is deprecated error.

@cortinico
Copy link
Member

Is this a regression of 1.23.1?

@TWiStErRob
Copy link
Member Author

@cortinico
Copy link
Member

Yup so this was introduced by: fa788eb

The workaround is to don't use OptionalWhenBraces at all

@TWiStErRob
Copy link
Member Author

TWiStErRob commented Jul 30, 2023

The workaround is to don't use OptionalWhenBraces at all

Yeah, the only way to do that is to list all other rules, or use the default config, which means allRules + warningsAsErrors is an unsupported feature as of 1.23.1.

@cortinico
Copy link
Member

which means allRules + warningsAsErrors is an unsupported feature as of 1.23.1.

Ah I see what happened. I don't know how do we want to handle this as practically we're deprecating a rule for the sake of 2.0.

So you will have a warning if you use that rule (also with allRules). At the same time warningsAsErrors will let your build fail because of that deprecated rule.

Perhaps allRules should skip deprecated rules?

@TWiStErRob
Copy link
Member Author

Perhaps allRules should skip deprecated rules?

I think that's a good one!

It assumes that "replacement" will be available, but I think that's a fair assumption.

@TWiStErRob
Copy link
Member Author

TWiStErRob commented Aug 9, 2023

@cortinico I think this should be fixed on 1.x, because right now it's not possible to upgrade from 1.23.0 to 1.23.1 (and future), unless:

  • we disable allRules
  • we disable warningsAsErrors

neither of which sound good as a workaround.


I had a quick look at "skipping deprecated rules", but the information is readily available in AllRulesConfig and I'm not 100% sure how to get it there. Maybe creating a Config.DEPRECATED_KEY and propagating the properties file into the config, or is that too hacky?


Little off topic: should we have a "next 1.x patch" milestone?

@cortinico
Copy link
Member

Yup we should fix this, I don't have the time to look into the ignore deprecated rules, but the easiest way for me would be to just keep a listOf(...) rule that we update manually as a filter for allRules.

That is needed only for 1.x anyway

Little off topic: should we have a "next 1.x patch" milestone?

As of now, I'm tracking them using the "pick request" label, but we can create the 1.23.2 milestone if helpful 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants