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

feat: allow running only a specific linter without modifying the file configuration #4438

Merged
merged 5 commits into from
Mar 6, 2024

Conversation

ldez
Copy link
Member

@ldez ldez commented Mar 2, 2024

This PR is a proposal for a new flag (not a file configuration).

This new flag will allow running only one specific linter without modifying the file configuration.

I named this flag --only but I'm not sure about this name.
I think --force can be ambiguous 🤔.

This flag will override all the options of the linters section from the file configuration (enable, disable, enable-all, disable-all, fast, and presets ).
So you can run a linter with the right linters-settings section from the file without modifying the file configuration.

Before:

golangci-lint run --no-config --disable-all --enable=yourlintername

After:

golangci-lint run --enable-only=yourlintername

# OR

golangci-lint run --no-config --enable-only=yourlintername

Related to #3453, #1188, #1972, #1272, #715

@ldez ldez added area: config Related to .golangci.yml and/or cli options area: CLI Related to CLI labels Mar 2, 2024
@ldez ldez requested review from bombsimon and Antonboom March 2, 2024 20:54
@ldez ldez added the enhancement New feature or improvement label Mar 2, 2024
@ldez ldez force-pushed the feat/only branch 2 times, most recently from 32be3cf to 852a237 Compare March 2, 2024 21:35
@bombsimon
Copy link
Member

I like this, but at the same time I also think it feels like a workaround. I know how the precedence of flags and config works but I think I'd still expect this behavior when I run golangci-lint run --disalbe-all --enable my-linter. It feels like a weird decision to have the configuration file overwrite the flags in this scenario, or rather having the --enable flag be additive.

Is this approach because you don't want to introduce a breaking change or do you think the current requirement of using --no-config is reasonable and want to keep it?

Since this feels by far like the most common use of --enable (no data, just guessing), what do you think about having the command line --enable flag overwrite the configuration and if you want to add a single extra linter to that list you'd use a different flag such as --also-enable or --enable-add?

@ldez
Copy link
Member Author

ldez commented Mar 3, 2024

I like this, but at the same time I also think it feels like a workaround.
...
Is this approach because you don't want to introduce a breaking change or do you think the current requirement of using --no-config is reasonable and want to keep it?

It's not a workaround or something to avoid breaking changes: during the refactor I spend time thinking about the behavior of the flags.
I think now that the append is the right behavior even if it feels weird.

I will share my notes.


For people that use enable-all + disable

  • Using -D
    • as an override seems not positive because all previous disabled linters will be enabled.
    • as an append is interesting because it allows disabling more linters.
  • Using -E is useless in this context because you cannot enable and disable the same linter.

For people that use disable-all + enable

  • Using -E
    • as an override is interesting because it allows one to run only one linter and use the linters settings of the file. (no need to use --no-config)
    • as an append is useful when you can quickly test a linter but there is no linter configuration, so it's limited.
      • maybe some users use it as a way to have a set of linters in one place and another set in another place.
  • Using -D is useless in this context because you cannot enable and disable the same linter.

For people that only use enable without enable-all or disable-all (only default linters + the ones defined by enable)

  • Using -E
    • as an override is interesting because it allows one to run only one linter and use the linters settings of the file. (no need to use --no-config)
    • as an append is useful when you can quickly test a linter but there is no linter configuration, so it's limited.
      • maybe some users use it as a way to have a set of linters in one place and another set in another place.
  • Using -D seems not interesting because it just allows disabling default linters.

For people that only use disable without enable-all or disable-all (only default linters - the ones defined by disable)

  • Using -D
    • as an override is ?
    • as an append is ?
  • Using -E
    • as an override is ?
    • as an append is ?

For people that use enable and disable without enable-all or disable-all (only default linters + the ones defined by enable - the ones defined by disable)

  • Using -D
    • as an override is ?
    • as an append is ?
  • Using -E
    • as an override is ?
    • as an append is ?

What about interaction with --fast and presets?


As you can see, I didn't evaluate all the cases, because I think that users expected consistency between flags behavior: if -E override, then -D should override.
But using -D as an override is not something expected.
In the end, my conclusion was: that we just need a flag to override the list of enabled linters in some situations.

@bombsimon
Copy link
Member

But using -D as an override is not something expected.

Yeah really good point, that makes a lot of sense!

I agree that having only one of -E and -D be additive would be weird. What i think feels like the odd behavior is that --disable-all --enable doesn't actually disable all. I think it makes sense when not specifying --enable because by just specifying --disable-all the enable list needs to be grabbed from somewhere and that should be either the configuration file or flag.

I'm not sure if not using disable-all + enable list is widely used but I wouldn't have guessed so. That means that if you have disable-all + enable list in your config, surely --enable should be additive. But when I explicitly say "hey disable everything and enable this linter", I still get the list from the config. But at this point it's more a question of what to name flags and put the logic. --only is a nice and simple fix with little code. I think what I'm arguing for here is something like --disalbe-all-including-my-enable-list-in-theconfig (not actually this name but you get the point). The result will be the same of only one enabled linter. So maybe I'm more thinking about the confusion of why --disable-all --enable works the way it does more so than solving the issue you're doing with this PR.

I think your notes are really good and I'm probably biased. I've never used -E or -D for other than troubleshooting or examples in issues and discussions here on GitHub so I'm kinda used to the --no-config --disable-all --enable pattern from that. I see no reason making this more complicated so let's add this new flag and keep any old behavior!

@ldez ldez marked this pull request as ready for review March 3, 2024 15:25
@ldez ldez added this to the next milestone Mar 4, 2024
@ldez ldez changed the title feat: The Only Flag feat: allow running only a specific linter without modifying the file configuration Mar 5, 2024
pkg/commands/flagsets.go Outdated Show resolved Hide resolved
pkg/config/loader.go Show resolved Hide resolved
pkg/commands/flagsets.go Outdated Show resolved Hide resolved
pkg/config/loader.go Outdated Show resolved Hide resolved
pkg/config/loader.go Outdated Show resolved Hide resolved
@ldez ldez merged commit 803970f into golangci:master Mar 6, 2024
12 checks passed
@ldez ldez deleted the feat/only branch March 6, 2024 19:43
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 this pull request may close these issues.

4 participants