RFC: Use golangci-lint for linting #1323
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1323 +/- ##
==========================================
+ Coverage 97.62% 97.77% +0.14%
==========================================
Files 51 51
Lines 3366 3366
==========================================
+ Hits 3286 3291 +5
+ Misses 69 65 -4
+ Partials 11 10 -1
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| with: | ||
| go-version: ${{ matrix.go }} | ||
| cache: true |
There was a problem hiding this comment.
this is the default
|
I'm on board, I think using golangci-lint is a better idea than rolling more custom lint execution. I can't think of any downsides. I think the more interesting bit is which linters to enable. At a minimum, I'd use I think we should also not disable Thoughts? |
|
We're mostly in agreement on this:
|
|
Agreed in general.
I thought that it did not use defaults if you explicitly enumerated linters, but maybe that's historical behavior or maybe I'm conflating with something else. Either way, we should definitely enable all of them.
Yeah, not saying it should be done in this PR - I meant that ideally we (1) run with the intended config before landing the change, (2) fix/PR lint issues, then (3) merge the linter change. I don't think it's a huge deal, I just think that it adds more impetus to address errcheck failures than disabling the linter does. I also haven't run it on the repo yet so I'm not sure how widespread of a problem it is (not advocating for blocking this on a massive/annoying undertaking).
Me either! But I don't think we'd enable linters that we didn't intend to follow (and in general, I would imagine the unavoidable cases where we need |
No, the defaults are enabled by default. 'enabled' and 'disabled' are extras on top of that.
It's a bit: I would've fixed them in the same PR with a separate commit if it was a small handful. |
|
Yeah, that's too many. Mostly tests though, which at least is better than the other way around. :) |
|
Will await another maintainer's opinion before merging. |
There was a problem hiding this comment.
I'm not the biggest fan of golangci-lint, but it's also become the standard so I think it makes sense to use. That said, the default linters can change with updates, and can introduce either linters with false positives, or remove a linter previously being relied on for correctness, so in recent usages, we've been very specific about which linters we want enabled rather than relying on the default.
Fair. Yeah, the revive unused-param check is one such example. |
Use golangci-lint instead of bespoke linter setup. Adds exclusions based on current practices.
Fixes the following issues reported by revive: ``` logger.go:291:2: redefines-builtin-id: redefinition of the built-in function copy (revive) sink_test.go:71:2: redefines-builtin-id: redefinition of the built-in function close (revive) writer.go:51:2: redefines-builtin-id: redefinition of the built-in function close (revive) writer.go:63:2: redefines-builtin-id: redefinition of the built-in function close (revive) ```
Instead of hand-managing and running linters, use golangci-lint. Along with the golangci-lint defaults, enable a couple other linters we generally agree with. See also uber-go/zap#1323 for a similar change. As a result of this, we can: - Drop the dependabot for tools - Run the lint job in parallel with build/test - Simplify the Makefile
Instead of hand-managing and running linters, use golangci-lint. Along with the golangci-lint defaults, enable a couple other linters we generally agree with. See also uber-go/zap#1323 for a similar change. As a result of this, we can: - Drop the dependabot for tools - Run the lint job in parallel with build/test - Simplify the Makefile
Instead of hand-managing and running linters, use golangci-lint. Along with the golangci-lint defaults, enable a couple other linters we generally agree with. See also uber-go/zap#1323 for a similar change. As a result of this, we can: - Drop the dependabot for tools - Run the lint job in parallel with build/test - Simplify the Makefile
Instead of hand-managing and running linters, use golangci-lint. This simplifies the Makefile and allows lint to run in parallel with the build/test job. Along with the golangci-lint defaults, enable a couple other linters we generally agree with. See also uber-go/zap#1323 and uber-go/sally#121 for similar changes.
Instead of hand-managing and running linters, use golangci-lint. Along with the golangci-lint defaults, enable a couple other linters we generally agree with. See also uber-go/zap#1323 for a similar change. As a result of this, we can: - Drop the dependabot for tools - Run the lint job in parallel with build/test - Simplify the Makefile
Instead of home-rolled linting workflows, use golangci-lint to manage linting. See also uber-go/zap#1323
This PR switches to using golangci-lint to run linters. staticcheck is included and enabled by default. We add revive and a few others. All issues reported by the linters have been fixed. This also allows the linter to run in parallel with the tests. License header check has also been moved to golangci-lint, similarly to uber-go/fx#1157. Refs uber-go/fx#1150 Refs uber-go/zap#1323
This PR switches to using golangci-lint to run linters. staticcheck is included and enabled by default. We add revive and a few others. All issues reported by the linters have been fixed. This also allows the linter to run in parallel with the tests. License header check has also been moved to golangci-lint, similarly to uber-go/fx#1157. Refs uber-go/fx#1150 Refs uber-go/zap#1323
This is more of a proposal than just a PR.
Zap has been maintaining its own bespoke linter setup for a while
and it's starting to show its age.
For example, the upgrade in #1289 can't go through
without either making a change which we disagree with,
or configuring revive separately to ignore that linter check.
This PR switches to using golangci-lint to run linters.
staticcheck is included and enabled by default.
For revive, this adds the relevant opt-outs with adequate justification.
It also enables a couple govet linter checks
that would otherwise be difficult to enable.
The result also simplifies our Makefile greatly
with the minor added complexity that it assumes golangci-lint
is already installed.
Another advantage of this setup is that lint runs in a separate parallel CI job
while the tests are doing their thing.