Bump golangci-lint to v1.62.0 #3196
Conversation
gaby
requested review from
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
gaby
changed the title
WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on updates to error handling and linting directives. Key modifications include an increment in the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3196 +/- ##
==========================================
- Coverage 82.76% 82.72% -0.05%
==========================================
Files 114 114
Lines 11149 11149
==========================================
- Hits 9228 9223 -5
- Misses 1521 1525 +4
- Partials 400 401 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (7)
middleware/cache/heap.go (1)
Line range hint
82-82: Remove unnecessary errcheck directive.Similar to the
Pushmethod, theerrcheckdirective is unnecessary here as type assertions don't return an error value.- x := heap.Remove(h, realIdx).(heapEntry) //nolint:forcetypeassert,errcheck // Forced type assertion required to implement the heap.Interface interface + x := heap.Remove(h, realIdx).(heapEntry) //nolint:forcetypeassert // Forced type assertion required to implement the heap.Interface interface🧰 Tools
🪛 golangci-lint
44-44: directive
//nolint:forcetypeassert,errcheck // Forced type assertion required to implement the heap.Interface interfaceis unused for linter "errcheck"(nolintlint)
🪛 GitHub Check: codecov/patch
[warning] 44-44: middleware/cache/heap.go#L44
Added line #L44 was not covered by testsmiddleware/cache/manager.go (1)
Error handling should be implemented for storage operations in middleware packages
Based on the verification results, there are multiple instances of unhandled storage errors in middleware packages (cache, csrf, limiter) that need attention:
middleware/cache/manager.go: Unhandled errors in Get, Set, and Delete operationsmiddleware/csrf/storage_manager.go: Unhandled errors in Get, Set, and Delete operationsmiddleware/limiter/manager.go: Unhandled error in Set operationThe Storage interface in
app.goclearly indicates that these operations can fail, and the mainctx.goproperly handles storage errors, showing the expected pattern. The middleware implementations should follow the same approach.🔗 Analysis chain
Line range hint
119-147: Consider implementing proper error handling for storage operations.There's a pattern of unhandled errors in storage operations marked with TODO comments. These operations could fail in production (e.g., if using Redis/database storage) and the current implementation silently ignores such failures.
Let's verify the storage interface implementation to understand potential error scenarios:
Consider implementing one of these strategies:
- Bubble up errors to the caller for handling
- Add logging for storage failures
- Implement retry mechanisms for transient failures
- Add metrics/monitoring for storage operation failures
Would you like me to propose a specific implementation for any of these approaches?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find the Storage interface definition and its implementations ast-grep --pattern 'type Storage interface { $$$ }' # Find all storage operation calls to see the error handling patterns rg 'storage\.(Get|Set|Delete)' -A 2Length of output: 3658
middleware/logger/default_logger.go (1)
34-34: Consider using a typed wrapper for atomic valuesTo improve type safety and eliminate the need for type assertions, consider creating a typed wrapper around
atomic.Valuespecifically for string timestamps. This would move the type safety checks to initialization time rather than runtime.Example approach for future consideration:
type AtomicString struct { value atomic.Value } func (a *AtomicString) Store(s string) { a.value.Store(s) } func (a *AtomicString) Load() string { return a.value.Load().(string) }Also applies to: 64-64
🧰 Tools
🪛 golangci-lint
34-34: directive
//nolint:forcetypeassert,errcheck // Timestamp is always a stringis unused for linter "errcheck"(nolintlint)
middleware/adaptor/adaptor.go (1)
166-166: Consider a more robust error handling approach.The current error handling uses direct type assertion which, while working, could be made more robust. Consider using
errors.Asfor safer error type checking:- if _, _, err := net.SplitHostPort(r.RemoteAddr); err != nil && err.(*net.AddrError).Err == "missing port in address" { //nolint:errorlint,forcetypeassert // overlinting + if _, _, err := net.SplitHostPort(r.RemoteAddr); err != nil { + var addrErr *net.AddrError + if errors.As(err, &addrErr) && addrErr.Err == "missing port in address" {This approach:
- Eliminates the need for linting suppressions
- Provides better type safety
- Follows Go's error handling best practices
🧰 Tools
🪛 golangci-lint
166-166: directive
//nolint:errorlint,forcetypeassert,errcheck // overlintingis unused for linter "errcheck"(nolintlint)
client/hooks_test.go (1)
257-257: Remove unnecessary nolint directiveThe
//nolint:testifylint // testdirective is unused and can be safely removed as the assertion is already using a recommended approach.- require.Equal(t, []byte(applicationJSON), req.RawRequest.Header.ContentType()) //nolint:testifylint // test + require.Equal(t, []byte(applicationJSON), req.RawRequest.Header.ContentType())🧰 Tools
🪛 golangci-lint
257-257: directive
//nolint:testifylint // testis unused for linter "testifylint"(nolintlint)
middleware/logger/logger_test.go (1)
308-308: LGTM! More efficient test assertion.The change improves the assertion by avoiding unnecessary string formatting when the test passes. The testify library handles the format string and arguments directly.
Consider applying this pattern to other test assertions in the codebase for consistency and efficiency.
ctx_test.go (1)
3552-3553: Enhance error assertion for unsupported JSON marshallingAttempting to marshal a complex number with
c.JSON(complex(1, 1))will produce an error since complex numbers are not supported by JSON. Consider adding an assertion to check the specific error message to ensure accurate error handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
.github/workflows/linter.ymlis excluded by!**/*.yml
📒 Files selected for processing (13)
Makefile(1 hunks)client/hooks_test.go(5 hunks)client/response_test.go(2 hunks)ctx_test.go(5 hunks)log/default.go(1 hunks)middleware/adaptor/adaptor.go(1 hunks)middleware/cache/heap.go(1 hunks)middleware/cache/manager.go(1 hunks)middleware/limiter/manager.go(1 hunks)middleware/logger/default_logger.go(2 hunks)middleware/logger/logger_test.go(2 hunks)middleware/logger/tags.go(1 hunks)router.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint
client/hooks_test.go
257-257: directive //nolint:testifylint // test is unused for linter "testifylint"
(nolintlint)
439-439: directive //nolint:testifylint // test is unused for linter "testifylint"
(nolintlint)
log/default.go
96-96: directive //nolint:forcetypeassert,errcheck // Keys must be strings is unused for linter "errcheck"
(nolintlint)
middleware/adaptor/adaptor.go
166-166: directive //nolint:errorlint,forcetypeassert,errcheck // overlinting is unused for linter "errcheck"
(nolintlint)
middleware/cache/heap.go
44-44: directive //nolint:forcetypeassert,errcheck // Forced type assertion required to implement the heap.Interface interface is unused for linter "errcheck"
(nolintlint)
middleware/limiter/manager.go
48-48: directive //nolint:forcetypeassert,errcheck // We store nothing else in the pool is unused for linter "errcheck"
(nolintlint)
middleware/logger/default_logger.go
34-34: directive //nolint:forcetypeassert,errcheck // Timestamp is always a string is unused for linter "errcheck"
(nolintlint)
64-64: directive //nolint:forcetypeassert,errcheck // Timestamp is always a string is unused for linter "errcheck"
(nolintlint)
middleware/logger/tags.go
203-203: directive //nolint:forcetypeassert,errcheck // We always store a string in here is unused for linter "errcheck"
(nolintlint)
router.go
237-237: directive //nolint:errcheck // It is fine to ignore the error here is unused for linter "errcheck"
(nolintlint)
🪛 GitHub Check: codecov/patch
middleware/cache/heap.go
[warning] 44-44: middleware/cache/heap.go#L44
Added line #L44 was not covered by tests
🔇 Additional comments (16)
Makefile (2)
38-38: Additional recommendations for linting tool update.
Since this is a linting tool update, please ensure:
- The new version has been tested locally to verify it doesn't introduce unexpected linting errors
- CI pipeline passes with the updated version
- Team is aware of any new linting rules introduced in v1.62.0
38-38: Verify golangci-lint version update.
The version bump from v1.61.0 to v1.62.0 looks good. Let's verify this is the latest stable version.
✅ Verification successful
Version update to golangci-lint v1.62.0 is verified and appropriate
The version bump from v1.61.0 to v1.62.0 is confirmed to be the latest stable release. The changelog shows primarily dependency updates and minor improvements, including:
- New linters (iface, recvcheck)
- Multiple dependency version bumps
- Bug fixes for Go version detection and cross-compilation
- No breaking changes identified
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify golangci-lint version is latest stable
# Expected: v1.62.0 should be the latest release
# Check the latest release tag
gh release view --repo golangci/golangci-lint --json tagName --jq .tagName
# Get the changelog for v1.62.0 to understand changes
gh release view v1.62.0 --repo golangci/golangci-lint --json body --jq .body
Length of output: 5798
middleware/cache/heap.go (1)
44-44: Add test coverage for heap interface implementation.
The Push method is part of the heap.Interface implementation but lacks test coverage. Consider adding tests to verify the correct behavior of the heap operations.
Would you like me to help generate comprehensive test cases for the heap operations?
🧰 Tools
🪛 golangci-lint
44-44: directive //nolint:forcetypeassert,errcheck // Forced type assertion required to implement the heap.Interface interface is unused for linter "errcheck"
(nolintlint)
🪛 GitHub Check: codecov/patch
[warning] 44-44: middleware/cache/heap.go#L44
Added line #L44 was not covered by tests
middleware/cache/manager.go (1)
53-53: LGTM: Appropriate linter directive for safe type assertion.
The addition of errcheck to the nolint directive is correct since the type assertion is safe here - the pool only stores *item objects via its New function.
middleware/logger/tags.go (1)
203-203: Verify the timestamp storage pattern.
The comment claims "We always store a string in here" for the timestamp. Let's verify this claim across the codebase to ensure the type assertion is indeed safe.
✅ Verification successful
Type assertion for Timestamp.Load() is safe
The timestamp handling pattern is consistent and safe throughout the codebase:
- The
Timestampfield is stored asatomic.Valuein theDatastruct - All stores are performed using
time.Now().In(cfg.timeZoneLocation).Format(cfg.TimeFormat)which always returns a string - No other value types are stored in the Timestamp field
The comment "We always store a string in here" is accurate and the type assertion is safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all timestamp-related storage operations to verify the type consistency
echo "Searching for Timestamp storage patterns..."
rg -p "Timestamp\.Store\(" --type go
# Search for any potential timestamp type definitions
echo "Searching for Timestamp type definitions..."
ast-grep --pattern 'type $_ struct {
$$$
Timestamp $_
$$$
}'
Length of output: 803
Script:
#!/bin/bash
# Let's check where and how Timestamp is being set
echo "Checking Timestamp initialization and storage..."
rg -p "Timestamp\." -B 3 -A 3 --type go
# Let's also check for any time-to-string conversions related to timestamp
echo "Checking time string formatting patterns..."
rg "time\." --type go | grep -i "format"
Length of output: 1696
🧰 Tools
🪛 golangci-lint
203-203: directive //nolint:forcetypeassert,errcheck // We always store a string in here is unused for linter "errcheck"
(nolintlint)
client/response_test.go (3)
378-378: LGTM! Improved JSON comparison in file path test.
The change from require.Equal to require.JSONEq is a good improvement as it properly handles JSON equality comparison, making the test more robust against formatting differences while still ensuring structural and value equality.
399-399: LGTM! Improved JSON comparison in io.Writer test.
Similarly, using require.JSONEq instead of require.Equal for the buffer content comparison is the correct approach for JSON validation.
378-378: Verify consistent JSON comparison updates across test files.
The changes to use JSONEq for JSON comparisons appear to be part of a broader effort to improve test assertions. Let's verify this pattern in other test files.
Also applies to: 399-399
✅ Verification successful
Let me check the actual JSON comparison patterns in these files to determine if they need similar updates.
No consistency issues found with JSON comparisons
The codebase shows appropriate usage of both require.Equal and require.JSONEq:
require.JSONEqis correctly used for comparing JSON content/responsesrequire.Equalis used for other cases like content-type headers, simple string comparisons, or array representations- The current usage in
response_test.gofollows the established pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other test files that might need similar JSON comparison updates
echo "Searching for potential JSON comparison patterns in tests..."
rg -l 'require\.Equal.*json' --type go --glob '*_test.go'
echo "Searching for existing JSONEq usage..."
rg -l 'require\.JSONEq' --type go --glob '*_test.go'
Length of output: 462
Script:
#!/bin/bash
# Check the actual JSON comparison patterns in test files
echo "Checking JSON comparisons in ctx_test.go..."
rg 'require\.Equal.*json|require\.JSONEq' -B2 -A2 ctx_test.go
echo -e "\nChecking JSON comparisons in helpers_test.go..."
rg 'require\.Equal.*json' -B2 -A2 helpers_test.go
echo -e "\nChecking JSON comparisons in client/hooks_test.go..."
rg 'require\.Equal.*json' -B2 -A2 client/hooks_test.go
echo -e "\nChecking JSON comparisons in client/client_test.go..."
rg 'require\.Equal.*json' -B2 -A2 client/client_test.go
echo -e "\nChecking JSON comparisons in client/request_test.go..."
rg 'require\.Equal.*json' -B2 -A2 client/request_test.go
Length of output: 4977
client/hooks_test.go (3)
300-301: LGTM! Improved assertion clarity
The change from require.True to require.Contains is a good improvement as it:
- Provides more descriptive error messages on failure
- Better expresses the intent of checking for substring presence
492-493: LGTM! Improved assertion clarity
The change from require.True to require.Contains enhances test readability and provides better error messages.
505-507: LGTM! Well-structured assertions
The changes improve the test by:
- Using clearer
require.Containsassertions - Separating checks for boundary, content, and form data into distinct assertions
middleware/logger/logger_test.go (1)
352-352: LGTM! Consistent with the previous change.
The assertion follows the same efficient pattern as the previous change, maintaining consistency across the test file.
ctx_test.go (4)
1192-1194: Approved: Correct expected XML output in test assertion
The test case correctly asserts the expected XML output generated by the AutoFormat function. The use of require.Equal is appropriate here.
3561-3563: Approved: Correct Content-Type header with custom content type
The test sets a custom content type application/problem+json. The assertions correctly verify that the response body and Content-Type header are as expected.
3615-3617: Approved: JSON serialization in benchmark test
The benchmark test correctly serializes the provided struct into JSON and asserts the output. The implementation is efficient and appropriate for benchmarking purposes.
3638-3640: Approved: Use of custom JSON encoder in test
The custom JSON encoder is effectively used to return a specific JSON output, and the test verifies this output correctly. This demonstrates proper customization of the JSON encoding process.
|
CodeRabbitAI is using golangci-lint 1.61.0 instead of 1.62.0 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: a97913b | Previous: 7126e48 | Ratio |
|---|---|---|---|
Benchmark_Compress_Levels/Brotli_LevelBestCompression - B/op |
6 B/op |
0 B/op |
+∞ |
BenchmarkAppendMsgitem - MB/s |
3224.49 MB/s |
1846.77 MB/s |
1.75 |
This comment was automatically generated by workflow using github-action-benchmark.
Description
Type of change