Repository navigation
Add formatintitoa and osisnotexist Go linters - #70801
Conversation
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Commit pushed:
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
🏗️ Design Decision Gate — ADR RequiredThis PR adds 404 lines in 📄 Draft ADR committed: The draft records these decisions: add two standalone
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and check that the implementation matches the decision. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
No actionable blocking issues found in the changed lines. The analyzer implementations, package/type resolution, suppression/generated-file handling, registry updates, and regression fixtures are internally consistent. Static review only; repository tests were not executed.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · 3.39 AIC · ⌖ 0.543 AIC · ⊞ 1.6K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — commenting with minor consistency suggestions, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Minor message-format duplication in
osisnotexist'sIsTimeoutbranch, which bypasses the shared report template used for the other three predicates. - Inconsistent fix coverage:
formatintitoashipsSuggestedFixes, whileosisnotexistdoes not, even though most of its rewrites are mechanical. Likely intentional given the advisory-only rollout, but worth a short comment to document the decision.
Positive Highlights
- ✅ Both analyzers follow the existing
analyzerutil/astutil/filecheck/nolintconventions closely — no architectural drift. - ✅ Excellent test coverage: aliases, dot-imports, shadowed identifiers, generated-file skipping, and
(nolint/redacted)suppression are all exercised, and golden-fix fixtures validate the exact rewritten text forformatintitoa. - ✅ Both linters are correctly held out of CI enforcement with documented rationale in
doc_sync_test.gowhile 116 existing production findings are triaged — a sensible, low-risk rollout strategy. - ✅ Documentation (
README.md,doc.go,spec_test.go, registry) is updated consistently and thedoc_sync_test.gocross-checks keep these in sync automatically.
Note: pr-triage ran successfully and confirmed the high_impact_files reviewed here; no fallback heuristic was needed.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 98.5 AIC · ⌖ 15.1 AIC · ⊞ 10.2K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/linters/osisnotexist/osisnotexist.go:52
[/codebase-design] The IsTimeout message duplicates the "does not inspect arbitrary wrapped errors" phrase that the shared os.%s does not inspect arbitrary wrapped errors; use %s instead template below already encodes, instead of reusing it with a timeout-specific replacement string.
Content redacted to fit size limit.
@copilot please address this.
pkg/linters/osisnotexist/osisnotexist.go:57
[/codebase-design] Unlike its sibling formatintitoa in this same PR (and most of the other 80 analyzers in this package), osisnotexist ships no SuggestedFixes, even though IsNotExist/IsExist/IsPermission rewrites are largely mechanical (os.IsX(err) -> errors.Is(err, fs.ErrX)).
Content redacted to fit size limit.
@copilot please address this.
There was a problem hiding this comment.
🟡 Changes recommended
The integer-formatting autofix can remove an import’s last use and break compilation.
2 open issues
2 files not reviewed
pkg/linters/formatintitoa/testdata/src/formatintitoa/generated.go: Generated filepkg/linters/osisnotexist/testdata/src/osisnotexist/generated.go: Generated file
What changed in this PR
Extends gh-aw’s custom Go linters with integer-formatting simplification and wrapped-error-aware error checks.
Changes:
- Add
formatintitoaandosisnotexistanalyzers. - Add diagnostic and suggested-fix regression fixtures.
- Update registration and documentation while deferring CI enforcement.
| File | Description |
|---|---|
pkg/linters/spec_test.go |
Extend analyzer contracts. |
pkg/linters/registry.go |
Register both analyzers. |
pkg/linters/README.md |
Document both rules. |
pkg/linters/osisnotexist/testdata/src/osisnotexist/osisnotexist.go |
Cover predicates, aliases, and suppression. |
pkg/linters/osisnotexist/testdata/src/osisnotexist/generated.go |
Verify generated-file exclusion. |
pkg/linters/osisnotexist/testdata/src/dotimport/dotimport.go |
Cover dot imports. |
pkg/linters/osisnotexist/osisnotexist.go |
Detect legacy error predicates. |
pkg/linters/osisnotexist/osisnotexist_test.go |
Run diagnostic fixtures. |
pkg/linters/formatintitoa/testdata/src/formatintitoa/generated.go |
Verify generated-file exclusion. |
pkg/linters/formatintitoa/testdata/src/formatintitoa/formatintitoa.go.golden |
Define expected fixes. |
pkg/linters/formatintitoa/testdata/src/formatintitoa/formatintitoa.go |
Cover conversions and exclusions. |
pkg/linters/formatintitoa/testdata/src/dotimport/dotimport.go |
Cover dot imports and shadowing. |
pkg/linters/formatintitoa/testdata/src/alias/alias.go.golden |
Define import-alias fix output. |
pkg/linters/formatintitoa/testdata/src/alias/alias.go |
Cover aliased imports. |
pkg/linters/formatintitoa/formatintitoa.go |
Detect verbose formatting and offer fixes. |
pkg/linters/formatintitoa/formatintitoa_test.go |
Run diagnostic and golden-fix fixtures. |
pkg/linters/doc.go |
Update analyzer catalog. |
pkg/linters/doc_sync_test.go |
Record deferred enforcement. |
Review effort: Max
| astutil.HasOverlappingComment(pass.Files, call.Pos(), call.End()) { | ||
| return nil | ||
| } | ||
| pkgText := astutil.NodeText(pass.Fset, selector.X) |
| nolint.HasDirectiveForLinter(pos, noLintIndex, "formatintitoa") { | ||
| return | ||
| } | ||
| pkgPath, name, ok := astutil.PackageCall(pass, call) |
Summary
Add the two requested Go linters to catch unnecessarily verbose integer formatting and legacy error checks that miss arbitrarily wrapped errors.
formatintitoarecommendsstrconv.Itoa(x)for decimalstrconv.FormatInt(int64(x), 10)calls wherexis anint. Suggested fixes handle parentheses and import/type aliases and are withheld when comments overlap or the call is indirect.osisnotexistflagsos.IsNotExist,os.IsExist,os.IsPermission, andos.IsTimeout. It recommends the appropriateerrors.Issentinel checks, but uses separateerrors.Asguidance for timeouts becauseos.ErrDeadlineExceededdoes not represent every timeout.Both analyzers remain outside CI enforcement while existing production findings are reviewed and remediated. The branch incorporates main through
84f7eee67e.Validation
Closes #70313
Closes #70579