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

Deprecate activateAllRules #5291

Closed
BraisGabin opened this issue Sep 10, 2022 · 3 comments
Closed

Deprecate activateAllRules #5291

BraisGabin opened this issue Sep 10, 2022 · 3 comments
Labels

Comments

@BraisGabin
Copy link
Member

The activateAllRules flag seems more a like a debug feature than a proper features. Enable all the rules by default is not a good call in general. We have a lot of controversial rules and we even have rules than do exactly the opposite: UseDataClass and ForbiddenPublicDataClass.

Because of this flag we also have really strange configurations on some rules (excludes: ['**'] #4940).

I still understand that some people want to start enabling ALL our rules and then disable the ones that they don't like. But I think that #5089 helps for that use case way better.

Also, once we remove this we will simplify our configuration code a bit more (#4926)

@BraisGabin BraisGabin changed the title Deprecate allRules Deprecate activateAllRules Sep 10, 2022
@TWiStErRob
Copy link
Member

TWiStErRob commented Sep 23, 2022

Just my two cents.

I (and I'm sure others too) do not want to list all the rules Detekt has, it's just noise in the configuration. It's much simpler to opt in to allRules, and disable whatever is not necessary. It's a lot of maintenance burden to add every new rule on every update rather than getting them "for free" on the cost of updating a number.

Compare default (as an example of a config that lists "almost" everything) vs my opt out configs.

This might change if #4925 is possible... because then creating the new config is not tedious (except if there are comments for the configured rules... then it's still hell to add new ones.)

Other analysis tools:

  • Android lint has the same config and it survived many majors, because it's a good config option.
    See also an example lint config which opts out of some that checkAllWarnings turns on automatically.
  • Java also has -Xlint:all.
  • Kotlin also turns on warnings by default (and there's no cherry-picking, except suppressWarnings=true.)

@arturbosch
Copy link
Member

Just some historical context: activateAllRules or failFast as it was called (all rules + all defaults) was one of the first and most wanted feature back in 2016/2017.
Basically with the same argument as @TWiStErRob stated. Get all rules + defaults and only deactivate ones you disagree with a custom config.
It is also the reason why we allow to specify more than one yaml config and layer them.

@BraisGabin
Copy link
Member Author

Both points have sense. Thanks for the feedback. Closing :)

@BraisGabin BraisGabin closed this as not planned Won't fix, can't repro, duplicate, stale Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants