Skip to content

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

galargh
Copy link
Contributor

@galargh galargh commented May 23, 2025

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:

  • when the workflow is triggered by the push event
  • when the pull request modifies either of
    • 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.

@galargh galargh force-pushed the uci-golangci-lint branch 5 times, most recently from b652f34 to 96fa173 Compare May 30, 2025 14:53
@galargh galargh force-pushed the uci-golangci-lint branch from 96fa173 to a96c3e1 Compare June 1, 2025 21:32
@galargh galargh force-pushed the uci-golangci-lint branch from 8f58fdd to 1b962cb Compare June 2, 2025 08:11
@galargh galargh force-pushed the uci-golangci-lint branch from 1b962cb to 3578f30 Compare June 2, 2025 08:17
@galargh galargh force-pushed the uci-golangci-lint branch from d9fd4a1 to 88d1e10 Compare June 2, 2025 08:51
@galargh galargh changed the title [DO NOT MERGE] wip: testing uci golangci-lint support chore: fix errors reported by golangci-lint Jun 3, 2025
@galargh galargh requested a review from sukunrt June 3, 2025 11:11
@galargh galargh marked this pull request as ready for review June 3, 2025 11:11
settings:
revive:
severity: warning
rules:
- name: unused-parameter
severity: warning
disabled: true
Copy link
Member

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)

Copy link
Contributor Author

@galargh galargh Jun 3, 2025

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

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

Successfully merging this pull request may close these issues.

2 participants