Expose the ability to enable all rules

[Zamiell]

Hello, and thanks for the linter. I have a feature request.

Right now, it appears that the only way that users can enable all rules and disable one rule is to manually specify every single existing rule into the config except for the one specific one that they don't want.

This is tedious, and not very future proof.
Meaning that when you guys add a new useful rule, I don't want to have to manually do research and find out which specific new rule that you added, and then manually add that new rule to my config in order to stay up-to-date. I just want to upgrade my version of golangci-lint and automatically get the latest and greatest linting without having to change my config.

As an analogy, golangci-lint allows this kind of workflow with the following YML config:

linters:
  enable-all: true
  disable:
    # These linters are deprecated
    - interfacer
    - maligned
    - scopelint

[chavacava]

Hi @Zamiell, thanks for filling the issue.

This is a subject that we had already talked about (for example #104 and #236) but for which we were not able to find a nice solution.

There are some aspects to take into account:

1. some rules need config (they will fail if not provided)...
2. ...using defaults config for those rules is possible but finding good defaults is hard (or impossible) because there are some very opinionated settings (for example: argument-limit, cyclomatic and the like)

[Zamiell]

1. some rules need config (they will fail if not provided)
2. ...using defaults config for those rules is possible but finding good defaults is hard

I think that both 1 and 2 are fine.

Consider the following hypothetical user story:

1. Alice decides to try out revive for the first time through golangci-lint. (She also disables golint, which she was previously using.)

2. Alice finds revive useful. But she realizes that she actually wants to disable the exported rule and keep everything else.

3. Alice adds the following to her .golangci.yml file:

linters-settings:
  revive:
    enable-all: true
    rules:
     - name: exported
       severity: disabled

4. Upon running the program again, Alice is greeted with an error message:
   rule A, B, and C need a default config and none was specified.

5. Based on their names, Alice knows that she doesn't actually care about rule A, B, or C. So instead of bothering to specify a config, she decides that it will be quicker to just disable them. Alice changes her .golangci.yml file again:

linters-settings:
  revive:
    enable-all: true
    rules:
     - name: exported
       severity: disabled
     - name: A
       severity: disabled
     - name: B
       severity: disabled
     - name: C
       severity: disabled

6. Upon running the program again, the error message is fixed! But now, Alice sees that rules D, E, and F generate a ton of noise across her code-base.

• This is partly because D and E are opinionated settings and finding good defaults for them is hard.
• F doesn't fall under this category. F has good defaults. But in this case, F is just a legitimately spurious rule that Alice doesn't care about.
  Alice changes her .golangci.yml file again to disable D, E, and F:

linters-settings:
  revive:
    enable-all: true
    rules:
     - name: exported
       severity: disabled
     - name: A
       severity: disabled
     - name: B
       severity: disabled
     - name: C
       severity: disabled
     - name: D
       severity: disabled
     - name: E
       severity: disabled
     - name: F
       severity: disabled

7. Now, revive is only generating a few messages, and these expose actual problems in Alice's codebase. The tuning process is over. Alice is satisfied with revive and decides to keep using it in the future.

8. Furthermore, Alice is happy that her "rules" config for revive is only around 14 lines long, which is quite minimal. It would be unwieldy and terrible if Alice had to copy paste 50+ rules into her config one by one, and then be burdened with keeping it up-to-date whenever the maintainers of revive decided to add another new rule.

---

One could argue that in this hypothetical, Alice is still burdened with keeping her config up-to-date, because as soon as the maintainers of revive add another annoying rule that she doesn't care about, then Alice has to go add yet-another-entry to her rule blacklist, and she is stuck in a cycle of updating it with more entries.

However, I don't think this is a valid criticism. Implicitly, by specifying enable-all: true, Alice is communicating that by default, she usually finds new linters/rules valuable. It is the exception rather than the norm when Alice adds a new rule to her blacklist. And in the hypothetical where Alice becomes exasperated by a torrent of new rules, she always has the freedom to remove the enable-all: true entry and "invert" her config, so to speak.

[chavacava]

@Zamiell, please check this branch and let me know if it fits your expectations.

The all-rules-activated configuration looks something like:

ignoreGeneratedHeader = false
severity = "warning"
confidence = 0.8
errorCode = 0
warningCode = 0
activateAllRules = true # here we activate all rules

# Deactivated rules
[rule.blank-imports]
    Deactivated = true
[rule.file-header]
    Deactivated = true
[rule.max-public-structs]
    Deactivated = true
[rule.line-length-limit]
    Deactivated = true
[rule.function-length]
    Deactivated = true

# Rule tuning
[rule.argument-limit]
    Arguments = [5]
[rule.cyclomatic]    
    Arguments = [10]
[rule.cognitive-complexity]    
    Arguments = [7]
[rule.function-result-limit]    
    Arguments = [3]

[Zamiell]

Nice, thanks chavacava.
Would it be simpler to use Severity = disabled rather than adding a whole new Deactivated = true?
I don't care much either way, just pointing that out.
If you do want to add a new field, then Disabled = true is a bit shorter and easier to type.

[chavacava]

Would it be simpler to use Severity = disabled rather than adding a whole new Deactivated = true?

Sure, it is simpler but disabled is not a severity (i.e. using the Severity field to deactivate a rule is somehow a hack)

If you do want to add a new field, then Disabled = true is a bit shorter and easier to type.

Yes, I've hesitated between Disable and Deactivate; even if I prefer Disable I've used Deactivate to be in line with the jargon already used in the code... later investigation shown that the activate jargon was introduced by me (7 month ago) therefore I will change to enable/disable :)

Meanwhile lets see what @mgechev says about all this.

[mgechev]

The process for introducing a tool for static analysis described in this comment sounds like:

• Enable all the rules
• Disable rules one by one until you get fewer errors/warnings
• Introduce the tool as part of your CI pipeline

That's probably a valid approach, especially in a large codebase. A more intentional approach I see is:

• Review the tool's documentation and find the rules which could be valuable in the project
• Enable them explicitly
• Fix all the errors/warnings
• Introduce the tool as part of your CI pipeline

Both have trade-offs. The pros of the first approach I see are:

• Preserves coincidental consistencies in the coding style in the long run
• Quick and easy to implement (no need to review docs, no need to fix warnings)

At the same time, I'm not convinced a developer should enable certain rule only because their code pass its checks.

Looking at golangci-lint's docs, looks like enable-all is deprecated because it doesn't scale well with frequent updates.

I'm not convinced we should move forward with this feature request. Let me know what your thoughts if you disagree.

[chavacava]

• Disable rules one by one until you get fewer errors/warnings
  ...
  At the same time, I'm not convinced a developer should enable certain rule only because their code pass its checks.

My understanding of the so called "first approach" is that a rule is disabled because it is not of interest (steps 5 and 6)
While implementing the proof-of-concept of the feature, I've went all over the steps to get a valid configuration. It took 8 iterations of: run revive, check output, fix config

Looking at golangci-lint's docs, looks like enable-all is deprecated because it doesn't scale well with frequent updates.

Yes, even if the enable-all config knob of golangci-lint applies to linters the reason of its deprecation might also apply to the activateAllRules knob we are discussing here.

I really like that activateAllRules allows to automatically take advantage of new rules (no need to review docs) the price is:

1. losing control over what rules are applied to your code base (i.e. your CI might start failing even if you did not change your code)
2. losing control on when you update the config (the addition of a new rule could force you to update the config)

Should we need to go on with this feature and let users decide to use it or not? Which is the price of adding this feature (config complexity, more constraints to implement future features) ?

[Zamiell]

At the same time, I'm not convinced a developer should enable certain rule only because their code pass its checks.

mgechev, it is unclear what you mean by this. For example, building on my hypothetical above:

• Alice has already gone through her first round of revive tuning. All of Alice's code passes the linter.
• Upon writing a new function, Alice realizes that her new code is failing the linter because of a previously-unknown rule that she had not come across before.
• Alice googles the rule, and realizes that it enforces an unwanted style.
• So, Alice goes ahead and promptly adds the rule to her blacklist, and then continues about her day.

Does this even count as a "disadvantage" of the workflow? That's just the normal, expected workflow.

Like, you could flip that logic on its head. If I may, I could rephrase your argument to the following: "A developer should not enable a rule if said rule has nothing to do with his codebase." But consider the following hypothetical:

• Bob decides to integrate golangci-lint into his Go project with the disable-all workflow.
• Bob researches every single golangci-lint linter included in the project. He compiles an exhaustive list of every linter that could possibly have to do with his code base, and then includes that in his config.
• Bob is very happy with golangci-lint. Time passes.
• 6 months later, as his program grows, Bob decides to add SQL functionality into his program.
• Bob makes the common mistake of forgetting to close a sql.Rows, causing a disastrous failure in production that causes Bob to be fired.
• Later on, Bob finds that there is actually a linter rule for this: sqlclosecheck, which is automatically included in golangci-lint. It's just that when Bob did his initial evaluation, he skipped over it, because his program didn't have any SQL in it.
• Bob resolves to always use the enable-all workflow instead of the disable-all workflow going forward.

Looking at golangci-lint's docs, looks like enable-all is deprecated because it doesn't scale well with frequent updates.

It actually might not be deprecated after all. See: golangci/golangci-lint#1888
Speciailly, Idez says "From what I can see in #803, #1686, and this issue, nobody seems to want the removal of enable-all."

[mgechev]

I really like that activateAllRules allows to automatically take advantage of new rules (no need to review docs) the price is:

losing control over what rules are applied to your code base (i.e. your CI might start failing even if you did not change your code)
losing control on when you update the config (the addition of a new rule could force you to update the config)

That's the main problem I see as well. Given the configuration is implicit by default, nobody really knows what checks apply to their project.

I don't see this as a scalable approach in large repositories.

Bob researches every single golangci-lint linter included in the project. He compiles an exhaustive list of every linter that could possibly have to do with his code base, and then includes that in his config.

This is the approach my colleagues and I would take + a team discussion after that. I consider it as a best practice.

Bob makes the common mistake of forgetting to close a sql.Rows, causing a disastrous failure in production that causes Bob to be fired.

Bob's company sounds like a bad place so he should have left on the first place. Looks like nobody reviewed his code and he was fired for a mistake anyone can make. Tools for static analysis, unfortunately, can't guarantee correctness and even if Bob somehow got lucky with the sql rules, he may hit a deadlock the next day and have the same fate.

Saying this, I'm not feeling strongly against the feature. revive is an open source project with many users so I'd suggest collecting opinions and making a decision based on this. Additionally, let's keep an eye on golangci/golangci-lint#1888.

[chavacava]

I think we can add the feature and let users decide to use it or not.

(I'll not use it but some of my colleagues find it useful)

[mgechev]

Sounds good!