chore(lint): enable stricter golangci-lint config and apply fixes#267
Open
appleboy wants to merge 3 commits into
Open
chore(lint): enable stricter golangci-lint config and apply fixes#267appleboy wants to merge 3 commits into
appleboy wants to merge 3 commits into
Conversation
- Upgrade go-anthropic to v2.20.1, genai to v1.57.0, promptkit to v0.11.0
- Upgrade Google API SDK, gRPC, OpenTelemetry, Charm libraries
- Upgrade fsnotify to v1.10.1, zalando/go-keyring to v0.2.8
- Upgrade golang.org/x/{crypto,net,sys,text}
- Enable errorlint, govet shadow, and additional gocritic checks - Wrap errors with %w and use errors.Is/As for comparisons - Rename shadowed err variables across cmd, git, and provider packages - Pass VersionInfo by pointer and range over slices by index - Preallocate slices and simplify empty-string and byte comparisons
There was a problem hiding this comment.
Pull request overview
This PR tightens the project’s GolangCI-Lint configuration and applies a set of lint-driven refactors across CLI/util/provider code (mostly around error handling, import grouping, and small idiomatic cleanups). It also updates a number of Go module dependencies.
Changes:
- Enabled additional GolangCI-Lint linters/settings (error handling, context, shadowing, goimports formatting, etc.).
- Refactored various call sites to use
errors.Is/As,%wwrapping, reduced shadowing, and small performance/idiom tweaks. - Updated
go.mod/go.sumwith several direct and indirect dependency upgrades.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
util/credstore.go |
Use errors.Is when handling credstore.ErrNotFound. |
util/credstore_test.go |
Align tests with errors.Is-based error checking. |
util/api_key_helper_test.go |
Avoid error variable shadowing in a test. |
proxy/proxy.go |
Use %w for wrapped errors in proxy setup. |
provider/openai/options_test.go |
Use errors.Is for error comparisons in tests. |
provider/openai/func.go |
Import grouping/order cleanup (goimports-compatible). |
provider/gemini/gemini.go |
Minor assignment style change to avoid shadowing. |
provider/anthropic/anthropic.go |
Avoid ranging-by-value over response content; avoid shadowing. |
git/git.go |
Preallocate slices, avoid string conversion for empty output, reduce shadowing. |
cmd/version.go |
Pass VersionInfo by pointer to avoid large-value copies. |
cmd/prompt.go |
Import grouping/order cleanup (goimports-compatible). |
cmd/hepler.go |
Import order + switch to %w in wrapped errors. |
cmd/commit.go |
Reduce shadowing and simplify string emptiness checks. |
cmd/cmd.go |
Use errors.As for viper config-not-found detection; reduce shadowing. |
go.mod / go.sum |
Dependency upgrades (direct + indirect). |
.golangci.yml |
Enable stricter linters, configure goimports/golines, tweak exclusions/settings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Close the *os.File returned by os.Create when creating an empty config - Clarify the goimports local-prefixes comment to reflect intent
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
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.
Summary
Fixes a batch of correctness bugs surfaced by a max-effort
/code-reviewof the whole codebase. The headline issues are two latent index-out-of-range panics in the hitting/pitching leaderboards and a live bug where tracked Athletics players never matched because the team's API abbreviation changed fromOAKtoATH.AI Authorship
/code-review --fixChange classification
What changed (by mechanism)
hitting.go,pitching.go):limitwas taken fromlen(stats[0])only, but the merge loop indexesstats[k][i]for every sort categoryk. Any later category returning fewer rows (or an empty200) causedindex out of range. Now bounded by the shortest column.daily_hitting.go): tracked entry{"OAK","Kurtz"}never matched — the live API returnsATH(verified). Changed toATH. This is the only user-visible behavior change.standings.go): season wastime.Now().Year()paired with an arbitrary--date, returning empty/wrong standings for cross-year queries. Season is now derived from the queried date.constant.go):HOA→HOU,OAK→ATH,SA→SD(verified against live API).abs.go): header usedfmt %-*s(rune-width) with a display-width value on CJK"比賽", misaligning columns; now usespadRight. Also guardedcolorNumagainst a negativestrings.Repeatcount.daily_pitching.go,daily_hitting.go): corrected two stale comments and dropped a redundantToLowerbeforeEqualFold.Verification
go test -count=1 ./...)go build,go vet,gofmt -lclean%-*sCJK width behaviormlb daily-hitting -d <date the A's played>and confirm a tracked A's player (e.g. Kurtz) is highlighted; runmlb standings -d <past-year date>and confirm the correct season is returned.Verifiability check
Risk & rollback
ATHchange alters which tracked players are highlighted. The season/alignment fixes only affect edge cases (cross-year dates, CJK header alignment).git revert 9adab3f— single self-contained commit.Reviewer guide
hitting.go/pitching.go(loop-bound logic) and the season derivation instandings.go.constant.go), comment fixes, and theToLowerremoval.Not fixed (surfaced, deferred by judgment)
FirstLastName/ShortNamemangle compound surnames ("De La Cruz"→"De") — no clean fix without a surname-particle list; better handled via a per-player override.getABS/fetchAllBoxscoresduplication, abbreviation-string vs team-ID matching, and several dead write-only fields — flagged in the review, left as follow-ups to avoid behavior/scope creep.