Modernize Go code per use-modern-go guidelines - #61
Conversation
Apply the gopls modernize analyzer (-fix -test) across all workspace modules, then a hand-reviewed sweep for guideline idioms the analyzer misses. All rewrites are mechanical and behavior-preserving; no public API changes. Analyzer: errors.AsType, range-over-int, slices.Contains/ContainsFunc/ Backward, atomic.Int32, reflect.TypeFor, new(bool) inline. Manual: sync.WaitGroup.Go, min/max clamp in BackoffDelay, strings. CutPrefix in decodeThreadID, strings.FieldsFuncSeq, json omitzero on bool/int wire fields. Validated: mise run vet + GOFLAGS=-race mise run test across all modules; burst/admission suites stressed with -race -count=5.
|
@codex review |
|
@codex security review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Guided modernization pass over all workspace modules, driven by the
use-modern-goskill installed in #60. Two layers: the goplsmodernizeanalyzer with fixes (go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -fix -test ./...per module), then a hand-reviewed manual sweep for skill-guideline idioms the analyzer misses. Every fix was reviewed against the guideline set for go 1.26 (run-tool.sh list). All rewrites are mechanical and behavior-preserving; no public API changes, no test-semantics changes. 22 files, +63/−109.State modules (
state/*) and example modules came back clean from the analyzer — findings were confined to the root module and adapters.Diff by idiom class
Analyzer-applied (hand-reviewed):
errors.AsType[T](go 1.26)linear.go/slack.gowebhook*http.MaxBytesErrorchecks + 6 test assertions; bound variable was unused everywhere, so_, ok :=formfor range nburst.gobatch loop + 6 test delivery loops; loop index still used inside bodiesslices.Contains/ContainsFunclookedUp,hasEvent,hasOutcome, eventually-conditions)slices.Backwardlinear/history.gonewest-first page reversalatomic.Int32history_hardening_test.gocounters (historyHits,handled)reflect.TypeFor[T]history_test.gocore-surface invariant checksnew(false)boolPtr(false); deleted the then-unusedboolPtrhelperManual sweep (guideline idioms the analyzer missed):
sync.WaitGroup.GoAdd(1)+go+defer Done()shape; loop-var capture safe per go ≥1.22 semanticsmin/maxbuiltinsBackoffDelayclamp →min(max(delay, retryAfter), maxDelay); required renaming the builtin-shadowing parammax→maxDelay(internal package, param rename invisible to callers)strings.CutPrefixdecodeThreadIDin both adapters (needed both trimmed rest and match bool)strings.FieldsFuncSeqparseGrantedScopesiterates fields directly; drops the intermediate slicejsonomitzerodirect bool,limit int,inclusive boolwire fields — encoding is byte-identical toomitemptyfor bool/int zerosDeliberately NOT modernized
t.Context()in tests — hardening tests encode context-lifetime invariants (detached/deferred dispatch must be independent of request contexts); swappingcontext.Background()fort.Context()changes what those tests assert. Left all of them.sync.OnceFunc/OnceValue— allsync.Onceuses (state/natsclose, close-channel-once test patterns) are direct.Docalls in control flow, not wrapper-closure memoization; converting adds nothing.time.Tick—runtime.godispatch/queue tickers have explicitStoptied to shutdown lifecycle; relying on GC recovery would weaken deliberate resource handling.omitemptyon string/slice/map/pointer/anyfields (~30) — thejson_omitzeroguideline itself says keepomitemptyfor empty strings/slices/maps; pointer/anyfields gain nothing fromomitzero.wg.Add(1)inadapters/lineartests (2 sites) —Done()is called inside event-handler callbacks registered with the bot, not theAdd+goshapewg.Goreplaces.cmp.Or— no true fallback chains; candidates were guard-and-panic (mustEnv) or two-expression branches.interface{},sort.Slice,time.Now().Sub(...), or bareerr ==sentinel comparisons in the tree.Validation
mise run vet— all 7 modules green.GOFLAGS=-race mise run test— all modules green (12 packageoklines incl. Testcontainers postgres/redis and embedded-NATS suites; zero FAIL/DATA RACE).go test -race -run 'Burst|Admission' -count=5 .green.modernize(no-fix) post-sweep: zero findings;gofmt -lclean.Generated with
mux• Model:anthropic:claude-fable-5• Thinking:xhigh