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

The --fast flag does not overwrite the configuration file #1909

Closed
2 of 4 tasks
rluders opened this issue Apr 12, 2021 · 8 comments · Fixed by #5475
Closed
2 of 4 tasks

The --fast flag does not overwrite the configuration file #1909

rluders opened this issue Apr 12, 2021 · 8 comments · Fixed by #5475
Labels
area: CLI Related to CLI area: config Related to .golangci.yml and/or cli options enhancement New feature or improvement

Comments

@rluders
Copy link

rluders commented Apr 12, 2021

  • Yes, I'm using a binary release within 2 latest major releases. Only such installations are supported.
  • 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 rluders added the bug Something isn't working label Apr 12, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Apr 12, 2021

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

@ldez ldez added enhancement New feature or improvement and removed bug Something isn't working labels Apr 12, 2021
@ldez
Copy link
Member

ldez commented Apr 12, 2021

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 ldez added bug Something isn't working enhancement New feature or improvement and removed enhancement New feature or improvement bug Something isn't working labels Apr 12, 2021
@ldez
Copy link
Member

ldez commented Apr 12, 2021

I found the explanation in the code:

// --fast removes slow linters from current set.
// It should be after --presets to be able to run only fast linters in preset.
// It should be before --enable and --disable to be able to enable or disable specific linter.

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 ldez added the blocked Need's direct action from maintainer label Apr 12, 2021
@miporto
Copy link

miporto commented Aug 24, 2021

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?

@Nivl
Copy link
Contributor

Nivl commented Aug 24, 2021

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.

@didrocks
Copy link

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.

@erolkskn
Copy link

erolkskn commented Dec 31, 2022

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.

@ldez
Copy link
Member

ldez commented Mar 4, 2025

Fixed by #5475

@ldez ldez closed this as completed Mar 4, 2025
@ldez ldez removed the blocked Need's direct action from maintainer label Mar 4, 2025
@ldez ldez removed this from the zfuture milestone Mar 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: CLI Related to CLI area: config Related to .golangci.yml and/or cli options enhancement New feature or improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants