chore(lint): adopt golangci-lint formatters and importas - #925
Merged
Conversation
Enables the two uber-go style-guide rules the linter set was missing —
"Import Grouping and Aliasing" and the stricter formatting behind "Group
Similar Declarations" — plus revive's "Exit in Main" check, and applies
the resulting reformat in the same commit so CI never sees a red tree.
Formatters are a TOP-LEVEL block in golangci-lint v2; listing gofumpt or
gci under linters.enable is a hard config error ("can't load config:
gofumpt is a formatter"). `golangci-lint run` still reports their output
as ordinary issues ("File is not properly formatted (gci)"), which is why
the reformat has to land here.
gci's section order is the load-bearing decision. Measured against the
pre-adoption tree at v2.12.2:
standard, default, prefix(go-bricks) -> 70 files / 290 lines
standard, default -> 138 files / 589 lines
standard, prefix(go-bricks), default -> 199 files / 806 lines
(the last needs custom-order: true; without it gci normalises back
to the first)
Prefix-last wins because most of the tree already separates third-party
from go-bricks in exactly that order by hand. Dropping the prefix section
is not merely 2x the churn: re-running the no-prefix order against the
formatted tree rewrites 197 files, merging third-party imports back into
the go-bricks block in every one. Do not "simplify" it later. goimports
is deliberately not enabled: it overlaps gci on grouping and the two
fight.
gofumpt runs with extra-rules: false. extra-rules was measured (+24
lines) and rejected — it implements no rule the uber-go guide asks for.
importas is a conflict-only map: five import paths the tree already
aliases two or more ways, each pinned to the majority alias (11 findings,
all fixed here). go.opentelemetry.io/otel/metric/noop is pinned to
metricnoop even though the plurality spelling was metricznoop — that is
a typo, and the two extra findings buy its deletion. net/http is
deliberately absent: 56 unaliased sites against 7 nethttp and 6 stdhttp
is not a convention to ratchet, it is a decision nobody has made yet.
no-unaliased and no-extra-aliases stay off; either turns an 11-site
cleanup into a ~40-file sweep. Already-consistent paths are left out
rather than added as ratchets, because a version-stamped path such as
otel/semconv/v1.NN.0 silently stops matching on the next bump.
cmd/seal-payload is excluded from the go-jose entry: it is the only file
set importing both go-jose/v4 (package name `jose`) and go-bricks/jose,
so one must be aliased away, and the go-bricks package is the dominant
one there. Keeping `gojose` on the vendor import is precisely the
collision-avoidance case uber-go's aliasing guidance prescribes.
revive gains `deep-exit`. It measured 0 findings and was verified to fire
against a planted os.Exit outside main/init, so it is a pure ratchet.
Note revive.rules REPLACES the default set, so the existing re-declared
defaults are untouched.
The Makefile change is not cosmetic. `make check` is `fmt lint ...`, and
`go fmt` cannot fix a gofumpt or gci finding — leaving it would reformat
and then fail lint on every run. Both Makefiles now call `golangci-lint
fmt` at the same pinned version their `lint` target uses. The two
subcommands also cover different file sets: `run` is package-scoped and
never loads the //go:build integration files, while `fmt` walks the tree
and does reach them — 5 needed reformatting.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (101)
💤 Files with no reviewable changes (5)
WalkthroughThe pull request standardizes Go linting and formatting. It updates repository commands and contributor guidance, then applies import ordering, alias naming, spacing, declaration grouping, signature formatting, and explicit octal syntax across source and test files. ChangesLint and formatting standardization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
The Makefile's new `fmt` comment claimed `golangci-lint run` is
package-scoped and never loads //go:build integration files, so only
`fmt` reaches them. Measured against this tree at v2.12.2 that is false:
planting a gci violation and then a gofumpt violation in
app/debug_integration_test.go, `golangci-lint run ./app/...` reports both
("File is not properly formatted"). The formatters see build-excluded
files under either subcommand. The practical consequence inverts the
original note — `run` (and therefore CI) fails on integration-file
formatting drift that only `fmt` can repair — so the comment now says
that instead.
CONTRIBUTING.md still told contributors `make fmt` formats with `go fmt`,
which this branch replaced with `golangci-lint fmt`.
No config or Go-source change: the .golangci.yml formatters/importas/revive
blocks and the ~100-file reformat are unchanged.
The `fmt` target's rationale claimed the formatters see //go:build integration files "under both subcommands", and concluded that `golangci-lint run` would fail on drift only `fmt` can repair. The opposite holds. Measured at the pinned v2.12.2 with one planted gofumpt violation (a blank line after a func's opening brace) in inbox/store_integration_test.go: golangci-lint fmt --diff <file> -> reports the diff, exit 1 golangci-lint run ./inbox/... -> 0 issues, exit 0 `run` is untagged here and in CI — ci-v2.yml has three lint jobs (lint-framework, lint-migrate-tool, lint-docs) and none passes -tags=integration — so it never loads those files at all. `fmt` is therefore the only thing keeping the five integration files formatted, which is a reason to keep this target, not evidence that `run` backstops it. The target itself is unchanged and its main justification still holds: `run` does report gofumpt/gci findings as ordinary issues on untagged files, so `go fmt` cannot sit here without leaving `make check` to fail its own lint step.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
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
1 participant
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.
What
Adopts the golangci-lint v2
formatters:layer (gofumpt,gci) for the uber-go guide's"Group Similar Declarations" and "Import Grouping and Aliasing" sections, plus a
conflict-only
importasmap (11 alias inconsistencies across 5 packages) and revive'sdeep-exit("Exit in Main", 0 findings). The reformat lands in the same commit becausegolangci-lint runenforces formatters as issues.Impact
None for consumers.
make fmtnow runsgolangci-lint fmtinstead ofgo fmt— required,since
go fmtcannot fix gofumpt/gci findings andmake checkwould reformat then faillint anyway. It also reaches 5
//go:build integrationfiles thatrunnever loads.Verification
The
gcisection order isstandard, default, prefix(go-bricks)deliberately: the naivestandard, defaultmerges third-party back into the go-bricks group across 197 files.Reformat is idempotent — a second
fmt --diffpass returns empty.Summary by CodeRabbit
Chores
Refactor
Tests