golangci-lint icon indicating copy to clipboard operation
golangci-lint copied to clipboard

The --fast flag does not overwrite the configuration file

Open rluders opened this issue 4 years ago • 7 comments

  • [X] Yes, I'm using a binary release within 2 latest major releases. Only such installations are supported.
  • [X] Yes, I've searched similar issues on GitHub and didn't find any.
  • [ ] Yes, I've included all information below (version, config, etc).
  • [ ] Yes, I've tried with the standalone linter if available. (https://golangci-lint.run/usage/linters/)
Description of the problem

It seems that the flag --fast kind of doesn´t overwrite the lining configuration that lives in the configuration file. At the same time, I would like to keep the file configurations, but at the same time, IMHO the flag --fast should have priority over the configuration file.

Version of golangci-lint
$ golangci-lint --version
# golangci-lint has version 1.39.0 built from 9aea4aee on 2021-03-26T08:02:53Z
Config file
$ cat .golangci.yml
You can try to use the one that is running on this own repository.
Go environment
$ go version && go env
# paste output here
Verbose output of running
$ golangci-lint cache clean
$ golangci-lint run -v
# paste output here
Code example or link to a public repository
// add your code here

rluders avatar Apr 12 '21 08:04 rluders

Hey, thank you for opening your first Issue ! 🙂 If you would like to contribute we have a guide for contributors.

boring-cyborg[bot] avatar Apr 12 '21 08:04 boring-cyborg[bot]

Hello,

The --fast works as expected with enable-all but with disable-all this option seems ignored.

Currently, I don't know if the behavior related to disable-all is expected. It's possible to think, as when you are using disable-all you have to use enable, that enable is a filter that overrides the --fast filter.

I need to think about it.

main.go
package main

import (
	"net/http"
)

func main() {
	println("foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo")

	req, err := http.NewRequest(http.MethodGet, "http://localhost:8080", nil)
	if err != nil {
		return
	}

	response, err := http.DefaultClient.Do(req)
	if err != nil {
		return
	}

	println(response.StatusCode)
}
disable-all
# .golangci.yml
linters:
  disable-all: true
  enable:
    - lll # fast
    - noctx # slow
$ golangci-lint run
main.go:9: line is 127 characters (lll)
        fmt.Println("foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo")
main.go:11:29: should rewrite http.NewRequestWithContext or add (*Request).WithContext (noctx)
        req, err := http.NewRequest(http.MethodGet, "http://localhost:8080", nil)
                                   ^
$ golangci-lint run --fast
main.go:9: line is 127 characters (lll)
        fmt.Println("foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo")
main.go:11:29: should rewrite http.NewRequestWithContext or add (*Request).WithContext (noctx)
        req, err := http.NewRequest(http.MethodGet, "http://localhost:8080", nil)
                                   ^
enable-all
# .golangci.yml
linters:
  enable-all: true
  disable:
    - scopelint
    - interfacer
    - maligned
$ golangci-lint run 
main.go:8: line is 123 characters (lll)
        println("foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo")
main.go:15:40: response body must be closed (bodyclose)
        response, err := http.DefaultClient.Do(req)
                                              ^
main.go:10:29: should rewrite http.NewRequestWithContext or add (*Request).WithContext (noctx)
        req, err := http.NewRequest(http.MethodGet, "http://localhost:8080", nil)
                                   ^
$ golangci-lint run --fast 
main.go:8: line is 123 characters (lll)
        println("foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo")

ldez avatar Apr 12 '21 16:04 ldez

I found the explanation in the code: https://github.com/golangci/golangci-lint/blob/93df6f75f528ad7ee83fbe28a22d2a70abe9ecdd/pkg/lint/lintersdb/enabled_set.go#L54-L56

But the explanation seems weird:

  • if you disable a linter, you don't need to remove it before with the fast filter because it will be removed
  • I don't understand the use case behind the fact to filter on fast linters then add a non-fast linter, sounds like a technical configuration and not a user configuration.

So IMHO, the fast must be applied after all the configuration settings.

@golangci/team we have 3 choices:

  • do nothing
  • change the current behavior in a minor release
  • change the current behavior in a major release

I think this change will not break any people because, for me, it's the expected behavior.

ldez avatar Apr 12 '21 18:04 ldez

Hi @ldez, how are you doing? With my team, we also use the fast filter combined with a configuration file that uses disable-all. Our use case is that we have a config file, but when we run golangci-lint in an IDE or code editor we want to filter the slow rules/linters out. Is this change being discussed internally? Do you know if this change was accepted and will be introduced?

miporto avatar Aug 24 '21 17:08 miporto

I don't understand the use case behind the fact to filter on fast linters then add a non-fast linter

Something I wish I could do but I assumed I couldn't (I haven't looked into it yet) is to have vscode load a project's config file , add --fast to it to remove all the slow linters, then add my own set of potentially slow linters.

The reason is that there are several linters that I think are useful, but I wouldn't use them on the CI and/or force them on other people (too opinionated, too many valid cases that break the rule, etc.). So the solution is to only enable them locally, and just for me.

Now, we probably all think that --fast overrides everything so I think we should go ahead and make --fast do what everyone think it's doing. If anything, we can add another option like --force-enable to deal with people like me. I do think it should be a major version though, since it will probably break something somewhere.

Nivl avatar Aug 24 '21 19:08 Nivl

Hi @ldez, how are you doing? With my team, we also use the fast filter combined with a configuration file that uses disable-all. Our use case is that we have a config file, but when we run golangci-lint in an IDE or code editor we want to filter the slow rules/linters out. Is this change being discussed internally? Do you know if this change was accepted and will be introduced?

We have the same exact use case. The configuration is shared between CI and IDE, but we want to filter for the IDE case "slow" linters we enabled for CI, so that we can only have one configuration file.

didrocks avatar Sep 16 '21 12:09 didrocks

It seems like it has been a while since its been discussed but this issue is still relevant and causing problems. (Like in: https://github.com/github/super-linter/issues/3596 https://github.com/github/super-linter/issues/143 )

@ldez IDK if you guys discussed it yet but here is an opinion on the case:

It seems like there are some negative opinions about how this kind of change can be breaking. How about creating a flag --fast-override-enabled or (--fast-override-all) that overrides enabled linters at the end of the process if --fast option is also given ? This way it will not be a breaking change and repositores which rely on the current behavior of --fast still can function as expected. This can be released in a minor release since it will be a new feature which is backward compatible rather than being a breaking change.

erolkskn avatar Dec 31 '22 14:12 erolkskn