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

Remove --exclude-use-default, default to false, or trim it WAY down #2239

Closed
natefinch opened this issue Sep 17, 2021 · 6 comments
Closed

Remove --exclude-use-default, default to false, or trim it WAY down #2239

natefinch opened this issue Sep 17, 2021 · 6 comments
Labels
duplicate This issue or pull request already exists enhancement New feature or improvement

Comments

@natefinch
Copy link
Contributor

natefinch commented Sep 17, 2021

Your feature request related to a problem? Please describe.

It took me way too long to figure out why golangci-lint wasn't telling me about so many linting errors that I could see with my own eyes.

This flag, which defaults to on, silently suppresses many important linting checks, and overrides the whole rest of your linting configuration in a very non-intuitive way.

The errors this setting suppress are, in many cases, extremely important to code quality.

Warning against using unsafe? That's important.

Warning against creating files and directories with permissions that are too open? That's important.

Warning about missing comments? That helps keep our codebase quality up, and is even mentioned in Effective Go. The comment warnings are especially egregious, in my opinion. The flag says "the rare codebase has such comments". That's an opinion, and I can't tell if it's saying that it's rare for people to write comments in the officially way (i.e. match the first word to the thing you're documenting), or if it's saying it's rare for people to not do that. But either way, it's certainly not rare.

This doesn't feel like a list of actual edge cases and false positives (for the most part), this feels like a list of one person's opinions about what kinds of linting errors matter. But that's what the configuration file is for.

Please reconsider this flag. I have seen what happens when you don't have sufficient linting combined with inexperienced devs... and the fact that the most popular linting program in the Go community is suppressing so many linting errors that are not edge cases, is extremely distressing.

      --exclude-use-default            Use or not use default excludes:
 # EXC0001 errcheck: Almost all programs ignore errors on these functions and in most cases it's ok
 - Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*print(f|ln)?|os\.(Un)?Setenv). is not checked

 # EXC0002 golint: Annoying issue about not having a comment. The rare codebase has such comments
 - (comment on exported (method|function|type|const)|should have( a package)? comment|comment should be of the form)

 # EXC0003 golint: False positive when tests are defined in package 'test'
 - func name will be used as test\.Test.* by other packages, and that stutters; consider calling this

 # EXC0004 govet: Common false positives
 - (possible misuse of unsafe.Pointer|should have signature)

 # EXC0005 staticcheck: Developers tend to write in C-style with an explicit 'break' in a 'switch', so it's ok to ignore
 - ineffective break statement. Did you mean to break out of the outer loop

 # EXC0006 gosec: Too many false-positives on 'unsafe' usage
 - Use of unsafe calls should be audited

 # EXC0007 gosec: Too many false-positives for parametrized shell calls
 - Subprocess launch(ed with variable|ing should be audited)

 # EXC0008 gosec: Duplicated errcheck checks
 - (G104|G307)

 # EXC0009 gosec: Too many issues in popular repos
 - (Expect directory permissions to be 0750 or less|Expect file permissions to be 0600 or less)

 # EXC0010 gosec: False positive is triggered by 'src, err := ioutil.ReadFile(filename)'
 - Potential file inclusion via variable

 # EXC0011 stylecheck: Annoying issue about not having a comment. The rare codebase has such comments
 - (comment on exported (method|function|type|const)|should have( a package)? comment|comment should be of the form)

 # EXC0012 revive: Annoying issue about not having a comment. The rare codebase has such comments
 - exported (.+) should have comment( \(or a comment on this block\))? or be unexported

 # EXC0013 revive: Annoying issue about not having a comment. The rare codebase has such comments
 - package comment should be of the form "(.+)...

 # EXC0014 revive: Annoying issue about not having a comment. The rare codebase has such comments
 - comment on exported (.+) should be of the form "(.+)..."

 # EXC0015 revive: Annoying issue about not having a comment. The rare codebase has such comments
 - should have a package comment, unless it's in another file for this package
(default true)

Describe the solution you'd like.

Default this to false in the next release and mark it as deprecated, and then remove it in the release afterward. People that still want these suppressions can configure them themselves.

Describe alternatives you've considered.

Maybe remove all the exclusions that aren't truly edge cases. Maybe excluding test.Test from the stuttering linters is fine, but suppressing unsafe usage? Comments? Remove those from that list. But really, I feel like this is not a flag that should default to true at all, and if it exists, should only be edge cases.

Additional context.

No response

@natefinch natefinch added the enhancement New feature or improvement label Sep 17, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Sep 17, 2021

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

@ldez
Copy link
Member

ldez commented Sep 17, 2021

Duplicate of #456

@ldez ldez marked this as a duplicate of #456 Sep 17, 2021
@ldez ldez closed this as completed Sep 17, 2021
@natefinch
Copy link
Contributor Author

natefinch commented Sep 17, 2021

I'll move comments to the other issue.

@bhcleek
Copy link

bhcleek commented Sep 17, 2021

This is not a duplicate of #456. This is a more generalized issue than #456, which is only about the exclusion complaints about missing godoc.

adamdecaf added a commit to moov-io/infra that referenced this issue Sep 20, 2021
We will often want to tweak these CLI flags, so there needs to be
support for doing so.

Related: golangci/golangci-lint#2239
@natefinch
Copy link
Contributor Author

Actually, yeah, I think it's not really the same thing. This flag silently overrides the default behavior of several of the linters this project depends on. None of these issues should be suppressed by default. It's one thing to offer an easy flag people can set, it's another to set it for them.

Contrary to what was posted in the other issue, backwards compatibility doesn't always have to be forever. Lots of binaries make backwards incompatible changes. Heck, even the go tool has changed how it works, with gopath vs modules. Gopath used to be the default, and now modules is the default.

If you make the changes stepwise, it's not that hard. (and we're talking about a single flag going from default on to default off. That's not a big lift. People would have to update golangci-lint to get the new behavior, so by definition, they're already messing with their ci/linting and could add back the flag if they wanted to.

@bhcleek
Copy link

bhcleek commented Oct 2, 2021

@ldez can we open this back up since it's not a duplicate of #456?

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

No branches or pull requests

3 participants