Enable all golangci-lint linters by default#5445
Merged
negz merged 23 commits intocrossplane:masterfrom Mar 4, 2024
Merged
Conversation
It seems like new linters are being added with each new version of golangci-lint. I like the idea of opting out of new linters we don't like, rather than opting into new linters. I think there will be several that we'll want to opt out of, which I'll do in a following commit. Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
This takes all linters that are currnently returning warnings about our codebase and puts them in one of two categories. The first are linters we'd like to enable, but that will require updates for compliance. The second are linters we don't want to enable. Signed-off-by: Nic Cope <nicc@rk0n.org>
With its default configuration this linter just finds fmt.Print* calls, which are usually debug statements left in by accident. We had a few of these where we instead should have been using t.Log or k.Stdout. Signed-off-by: Nic Cope <nicc@rk0n.org>
The 'nilness' linter notices this in my editor but not in make reviewable for some reason. Err could never be nil here because we check err for nilness before the for loop. The err declaration inside the for loop is scoped to that loop. Signed-off-by: Nic Cope <nicc@rk0n.org>
They do the same thing, but calculate cognitive complexity differently. Notably gocognit considers simple switch statements less complex, but nested conditionals more complex. This removes the need for a lot of //nolint:gocyclo comments that we had just for long switches. I notice nolintlint seems to automatically be removing these no-op //nolint comments now too, which is great. Signed-off-by: Nic Cope <nicc@rk0n.org>
We try to avoid using init unless it's really necessary or idiomatic (e.g. registering Kubernetes API types with the scheme). Generally we don't want importing our packages to have magic side effects. Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
For now we only prevent importing test assertion libraries. In future we could use it to avoid other things (e.g. MPL licensed libraries). Signed-off-by: Nic Cope <nicc@rk0n.org>
This linter finds duplicate words, e.g. "the the". Signed-off-by: Nic Cope <nicc@rk0n.org>
This linter makes sure you don't embed a context in a struct. We allow this in our table-driven tests, where we use structs to capture the arguments to be passed to functions. Signed-off-by: Nic Cope <nicc@rk0n.org>
Lots of //nolint comments for this one, but it seems worth at least prompting folks to think about. Signed-off-by: Nic Cope <nicc@rk0n.org>
We often use global variables to represent data that is by convention constant, but that can't be constant because it's mutable. So there's a few nolint comments for this, but I think it's still good to discourage folks. Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
This linter warns for interface methods without named parameters. Signed-off-by: Nic Cope <nicc@rk0n.org>
This linter warns for using predeclared names, like new or append. Signed-off-by: Nic Cope <nicc@rk0n.org>
This linter warns when you assign a value that will never be used to a variable. Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
We do believe in what this linter is linting for, but in practice we only seem to be returning interfaces where we need to. Signed-off-by: Nic Cope <nicc@rk0n.org>
This linter checks that JSON struct tags are Go style camelCase. This is mostly what we want, especially for apis. Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
fd44cda to
3325955
Compare
negz
commented
Mar 4, 2024
Comment on lines
+29
to
+36
| # These are linters we'd like to enable, but that will be labor intensive to | ||
| # make existing code compliant. | ||
| - wrapcheck | ||
| - varnamelen | ||
| - testpackage | ||
| - paralleltest | ||
| - nilnil | ||
| - gomnd |
Member
Author
There was a problem hiding this comment.
I think these would be useful, but I'm not going to tackle them in this PR. They'd require quite a lot of work to make our existing code compliant (e.g. either wrapping or //nolint-ing all errors).
phisco
approved these changes
Mar 4, 2024
2 tasks
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of your changes
It seems like new linters are being added with each new version of
golangci-lint. I like the idea of opting out of new linters we don't like, rather than opting into new linters. This way we're more likely to notice new linters, whether we like them or not.I think there will be several that we'll want to opt out of. I'll add a commit for each with rationale.
I have:
make reviewableto ensure this PR is ready for review.Linked a PR or a docs tracking issue to document this change.Addedbackport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.