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

Let's discuss about the deprecation of enable-all #1888

Closed
ldez opened this issue Apr 4, 2021 · 17 comments · Fixed by #2039
Closed

Let's discuss about the deprecation of enable-all #1888

ldez opened this issue Apr 4, 2021 · 17 comments · Fixed by #2039
Labels
enhancement New feature or improvement

Comments

@ldez
Copy link
Member

ldez commented Apr 4, 2021

Is your feature request related to a problem? Please describe.

@jirfag in #803 said:

enable-all has the following advantage:

  1. convenient for users, I understand it.
  2. all newly added linters are automatically used by a lot of users.

But it has the following downside: when golangci-lint is updated users typically get some new linters enabled: it can be annoying, it can fail CI builds. Users should disable linters one by one in such cases.

I think long-term updates cost is more important than a one-time configuration cost.

Let's discuss it!

#803 (comment)

but there was no discussion, the PR was just merged.

Describe the solution you'd like

I would like to open a constructive debate about this deprecation.

Describe alternatives you've considered

Removes the deprecation of enable-all.

Additional context

#1686

@ldez ldez added the enhancement New feature or improvement label Apr 4, 2021
@frederikhors
Copy link

I agree with you and I opened #1887 because I don't know how to auto-update (not manually, error-prone) new linters in my config.

@ldez
Copy link
Member Author

ldez commented Apr 4, 2021

I think that the deprecation was related to Golangci-lint service and CI.

Golangci-lint service is down.

Now, the problem with enable-all is for people that use latest in their CI because locally I think there is no real problem.

IMO the real bad practice is to use latest in a CI, I think most of us know this sentence: friends don't let friends use latest.

So I think we have to document this bad practice instead of deprecating enable-all.

@ldez
Copy link
Member Author

ldez commented Apr 4, 2021

Also the linters them-self evolve and can either introduce new rules or update their rules.
Then even if you enable only some linters you still have a possibility to break your CI.
enable-all is just a specific case of this problem.

So IMO, the solution is still the same: don't use latest in a CI and use an explicit version of the tools that run in a CI.

@ldez
Copy link
Member Author

ldez commented Apr 4, 2021

From what I can see in #803, #1686, and this issue, nobody seems to want the removal of enable-all.

Maybe I am missing some use-cases, so please express your voice if you have a use-case that can justify this removal.

We will wait, for the moment I don't know how long, but we will take the time for the debate.

@frederikhors
Copy link

In my day to day work I need something warns me about mistakes. If I upgrade it I trust that something has changed for the better and that more mistakes of mine can be detected.

If I know that I don't care about some linter I can comment out it in my config.yml.

Am I wrong in thinking like this?

@ldez ldez changed the title Discussions about the deprecation of enable-all Let's discuss about the deprecation of enable-all Apr 4, 2021
@ldez ldez pinned this issue Apr 4, 2021
@ldez ldez added the pinned label Apr 4, 2021
@butuzov

This comment has been minimized.

@ldez

This comment has been minimized.

@butuzov

This comment has been minimized.

@Zamiell
Copy link
Contributor

Zamiell commented May 15, 2021

In my day to day work I need something warns me about mistakes. If I upgrade it I trust that something has changed for the better and that more mistakes of mine can be detected.
If I know that I don't care about some linter I can comment out it in my config.yml.

This matches my own thinking. Consider the case of Bob:

  • 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 going forward.

@frederikhors
Copy link

frederikhors commented May 15, 2021

@Zamiell I am Bob.

@Zamiell
Copy link
Contributor

Zamiell commented May 15, 2021

Condolences.

@bconway
Copy link

bconway commented May 15, 2021

While I doubt we really need more support of --enable-all in this thread, the use case I have used multiple times is as follows:

  • CI is set up with golangci-lint with specific linters/versions and is strictly enforced.
  • My development environment uses --enable-all to find new, useful linters as they are introduced. I keep a -D list that I accumulate and wipe out every few releases of golangci-lint, and then start the evaluation cycle over.

@ldez
Copy link
Member Author

ldez commented May 15, 2021

After ~40 days, we still have 0 voice to remove that flag.

Please express your voice if you have a use-case that can justify the removal of --enable-all.

I think we will wait until the end of May or the beginning of June.

@bombsimon
Copy link
Member

As I remember it, @jirfag is one of very few who wanted to deprecate the flag which was met with disappointment from users already back in 2019 when this was first mentioned. As for my opinion I want to keep the flag to be able to use enable-all for local development.

I can't imagine who would have a CI pipeline of such high importance this would be an issue but still automatically bumps version of golangci-lint (which is required for this to be an issue) and uses enable-all. It sounds like an extremely unlikely use case and probably nothing for the maintainers of golangci-lint to consider.

@philoserf
Copy link

philoserf commented May 26, 2021

--enable-all, which was a choice, was a feature. That it informed us each time something changed in any of the supported linters was a feature. Most arguments I have seen in support of its removal come down to use cases for which --enable-all was the wrong choice. Let's emphasize that word choice. Removing --enable-all does not fix improper use of the tool in CI. I see folks getting it wrong because they are operating at the limits of their understanding. For that, we lost the choice to use --enable-all.

@ernado
Copy link
Member

ernado commented May 27, 2021

This flag is useful for some cases like discovering new linters, so I don't want it to be removed until there is an alternative.
However, it still should not be used in examples and we should encourage users to explicitly specify enabled linters to reduce confusion while upgrading to new version.

Probably we can find a better solution that will do both things, allowing people to discover new useful linters and still not adding too much false-positives or conflicting linters? This can be some sort of managed linter preset, but it requires separate discussion and a bit off-topic.

@ldez
Copy link
Member Author

ldez commented Jun 2, 2021

After 2 months, the time has come to open a PR: #2039

🎉 🎉 🎉 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants