Conversation
appleboy
commented
Feb 26, 2026
- Add and configure multiple new linters and settings in the golangci-lint configuration, while removing some previously enabled linters
- Add fine-grained linter rules, revive linter rules, and specific issue limits in lint configuration
- Switch several for-loops in tests and code from explicit indexing to range-based iteration for improved readability
- Replace fmt.Errorf with errors.New for static error strings to standardize error construction
- Import the errors package and strconv where needed for the new error handling approach and int-string conversions
- Update test code comments for clarity about direct HTTP calls to the local server
- Add missing error imports in test files to support new error usage
- Improve code style and add additional configuration for code formatters
- Add comments explaining specific os.Exit return codes
There was a problem hiding this comment.
Pull request overview
This PR standardizes linting and error-handling conventions in the Go codebase by expanding/adjusting golangci-lint configuration and applying small refactors across runtime and test code.
Changes:
- Updated
.golangci.ymlto enable a new set of linters (depguard/revive/etc.), configure rules/settings, and enable additional formatters. - Refactored several counted
forloops to Go’s integerrangeform for readability. - Standardized construction of static errors by replacing
fmt.Errorf("...")witherrors.New("..."), and replacedfmt.Sprintf("%d", n)withstrconv.Itoa(n).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
.golangci.yml |
Adds/enables linters and config rules; adjusts formatter settings and issue limits. |
main.go |
Uses errors.New for static errors and strconv.Itoa for int→string conversion; clarifies an os.Exit nolint comment. |
callback_test.go |
Switches one static error to errors.New and adds clarifying comments on direct HTTP calls. |
filelock.go |
Refactors retry loop to integer range. |
filelock_test.go |
Refactors goroutine-spawning loop to integer range. |
pkce_test.go |
Refactors iteration loops to integer range. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
callback_test.go
Outdated
| port, state, | ||
| ) | ||
| resp, err := http.Get(callbackURL) //nolint:noctx,gosec | ||
| resp, err := http.Get(callbackURL) //nolint:noctx,gosec // test: direct GET to local server |
There was a problem hiding this comment.
This file uses //nolint:noctx,gosec, but .golangci.yml no longer enables noctx or gosec and nolintlint.allow-unused is false. That combination will cause golangci-lint to fail due to unused/unknown nolint directives. Either re-enable those linters or remove the noctx,gosec nolint (keep a plain comment if you still want the rationale).
| resp, err := http.Get(callbackURL) //nolint:noctx,gosec // test: direct GET to local server | |
| resp, err := http.Get(callbackURL) // test: direct GET to local server |
callback_test.go
Outdated
| port, state, | ||
| ) | ||
| resp, err := http.Get(callbackURL) //nolint:noctx,gosec | ||
| resp, err := http.Get(callbackURL) //nolint:noctx,gosec // test: direct GET to local server |
There was a problem hiding this comment.
This //nolint:noctx,gosec directive will be considered unused because noctx/gosec are not enabled in .golangci.yml while nolintlint.allow-unused=false, which will fail CI. Remove the nolint (or re-enable the linters).
| resp, err := http.Get(callbackURL) //nolint:noctx,gosec // test: direct GET to local server | |
| resp, err := http.Get(callbackURL) // test: direct GET to local server |
callback_test.go
Outdated
| port, | ||
| ) | ||
| resp, err := http.Get(callbackURL) //nolint:noctx,gosec | ||
| resp, err := http.Get(callbackURL) //nolint:noctx,gosec // test: direct GET to local server |
There was a problem hiding this comment.
This //nolint:noctx,gosec directive will be flagged by nolintlint as unused/unknown (since those linters are disabled), causing lint failure. Remove it (or re-enable noctx/gosec).
callback_test.go
Outdated
| port, state, | ||
| ) | ||
| resp, err := http.Get(callbackURL) //nolint:noctx,gosec | ||
| resp, err := http.Get(callbackURL) //nolint:noctx,gosec // test: direct GET to local server |
There was a problem hiding this comment.
//nolint:noctx,gosec is likely to fail nolintlint because noctx/gosec are not enabled in .golangci.yml and unused nolints are disallowed. Remove the nolint (or re-enable those linters).
callback_test.go
Outdated
| for range 2 { | ||
| go func() { | ||
| resp, err := http.Get(url) //nolint:noctx,gosec | ||
| resp, err := http.Get(url) //nolint:noctx,gosec // test: direct GET to local server |
There was a problem hiding this comment.
This //nolint:noctx,gosec will be reported as unused by nolintlint unless noctx/gosec are enabled. With the updated .golangci.yml it will break lint; remove it or re-enable the linters.
callback_test.go
Outdated
| port, state, | ||
| ) | ||
| resp, err := http.Get(callbackURL) //nolint:noctx,gosec | ||
| resp, err := http.Get(callbackURL) //nolint:noctx,gosec // test: direct GET to local server |
There was a problem hiding this comment.
This //nolint:noctx,gosec directive will be considered unused under the updated linter set (noctx/gosec disabled) with nolintlint.allow-unused=false. Remove it (or re-enable the linters) to avoid CI failure.
| nolintlint: | ||
| allow-unused: false | ||
| require-explanation: true | ||
| require-specific: true |
There was a problem hiding this comment.
With nolintlint.allow-unused: false and require-specific: true, any //nolint:<linter> must reference an enabled linter. The codebase currently contains //nolint:noctx,gosec (e.g., in callback_test.go), but noctx and gosec are not enabled here, so lint will fail. Either enable those linters again or remove/update those nolint directives.
- Add and configure several new linters in .golangci.yml, including depguard, forbidigo, and revive; provide custom linter and formatter settings - Remove some previously enabled linters and formatters, and refine test exclusion rules - Replace instances of fmt.Errorf with errors.New where formatting is unnecessary, improving error semantic usage - Use strconv.Itoa instead of fmt.Sprintf for integer to string conversion - Refactor retry and timeout values in filelock.go to use named constants for maintainability - Simplify for-loops over integer constants in tests to use range style - Replace explicit error assertions in tests with discard assignment for cleaner code - Add comments for suppressed linter warnings and clarify test-only usage - Minor refactoring and style improvements to tests for consistency and clarity Signed-off-by: appleboy <appleboy.tw@gmail.com>
- Remove inline nolint comments from test code for http.Get usage Signed-off-by: appleboy <appleboy.tw@gmail.com>