-
Notifications
You must be signed in to change notification settings - Fork 1.2k
chore: fix errors reported by golangci-lint #3295
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
base: master
Are you sure you want to change the base?
Conversation
b652f34
to
96fa173
Compare
96fa173
to
a96c3e1
Compare
8f58fdd
to
1b962cb
Compare
1b962cb
to
3578f30
Compare
d9fd4a1
to
88d1e10
Compare
This reverts commit a96c3e1.
settings: | ||
revive: | ||
severity: warning | ||
rules: | ||
- name: unused-parameter | ||
severity: warning | ||
disabled: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this disabled? This was the entire reason why we included revive.
See: #3269 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It produces a lot of errors at the moment. See https://github.com/libp2p/go-libp2p/actions/runs/15387329399/job/43288668727, for example (btw, 50 is the default cap on the number of reported issues). Do we want all these occurrences to be replaced with _
? If so, I can take care of it and reenable the rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer enabling this particular check. It is useful.
I'm also happy with having the "only new changes" option for linting as we currently do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"only new changes" makes sense in the context of a PR but I think that on a push to the default branch, we should check the entire codebase. Otherwise, one cannot really check if their PR is OK before pushing the changes because golangci-lint would always fail locally due to a huge number of pre-existing failures.
If you don't mind, I'll try to fix the errors reported by the errcheck in the entire codebase then and then I'll reenable the check here.
This PR prepares go-libp2p for the addition of native golangci-lint support to UCI. I fix a bunch of errors reported by golangci-lint in this codebase and disable errcheck as it produces a huge number of errors and #3269 mentioned that there was some doubt whether it was going to be useful going forward.
Why the errors fixed here weren't reported by the current implementation of golangci-lint in CI?
This is because the added action was configured to check only the diff from the pull request i.e. it never runs checks over the entire codebase.
Unlike the default golangci-lint action, I propose the UCI implementation to run checks over the entire codebase in these cases:
go.mod
.github/workflows/go-check.yml
.golangci-lint.yaml
Otherwise, it will behave the same as the action we use now and check only the diff.
Are there any other differences between the UCI implementation of golangci-lint and the action we use now?
Yes! The action we use now does not run checks in subpackages such as examples. The UCI implementation does.
Another difference is that the UCI implementation runs staticcheck and govet separately to golangci-lint. And when we run golangci-lint in CI, we exclude these linters. When you run golangci-lint without these exclusions locally, you might get slightly different results due to different versions of these tools being used. In the future, I'd like to explore using staticcheck and govet from golangci-lint to bridge this gap.
Finally, the UCI implementation of golangci-lint will only run the checks on linux. It won't run them with different GOOS settings. Is that OK?
How will UCI implementation of golangci-lint get enabled?
After UCI version with that feature is released,
go-check
will start running the golangci-lint step on ubuntu-latest if it finds a golangci-lint configuration file in the repository. We'll be able to remove the jobs that currently run golangci-lint here then.