fix(lint): golang-client lint errors #3924
Conversation
kobakaku
requested review from
rach-id,
evan-forbes and
a team
as code owners
📝 WalkthroughWalkthroughThe pull request introduces several changes across multiple files, primarily focusing on updating linter configurations, enhancing test coverage, and improving error handling. Key modifications include the removal of deprecated linters, the addition of new test cases for transaction prioritization and data availability headers, and adjustments to error message formatting in the keeper package. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
x/blobstream/keeper/keeper_data_commitment.go (2)
139-139: Improved error handling and suggestion for consistencyThe change from using
fmt.Sprintfto directly formatting the error message inerrors.Wrapfis a good improvement. It makes the code more concise and addresses the lint error related to non-constant format strings inerrors.Wrapfcalls.For consistency, consider updating the error message to match the style used in the
GetLatestDataCommitmentmethod:- return false, errors.Wrapf(types.ErrAttestationNotFound, "nonce %d", i) + return false, errors.Wrapf(types.ErrAttestationNotFound, "nonce %d", i)
Line range hint
9-9: Address TODO: Add unit tests for all keepersThe TODO comment indicates that unit tests are missing for the keepers. While this is not directly related to the current PR's objectives, addressing this would significantly improve the code quality and maintainability.
Would you like assistance in creating a GitHub issue to track the task of adding unit tests for all keepers? This could help ensure that this important task is not overlooked in future development efforts.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- .golangci.yml (1 hunks)
- app/test/priority_test.go (0 hunks)
- pkg/da/data_availability_header_test.go (0 hunks)
- x/blob/client/testutil/integration_test.go (0 hunks)
- x/blobstream/keeper/keeper_data_commitment.go (2 hunks)
- x/blobstream/keeper/keeper_valset_test.go (0 hunks)
- x/mint/client/testutil/suite_test.go (0 hunks)
- x/mint/simulation/decoder_test.go (0 hunks)
💤 Files with no reviewable changes (6)
- app/test/priority_test.go
- pkg/da/data_availability_header_test.go
- x/blob/client/testutil/integration_test.go
- x/blobstream/keeper/keeper_valset_test.go
- x/mint/client/testutil/suite_test.go
- x/mint/simulation/decoder_test.go
🔇 Additional comments (3)
.golangci.yml (2)
7-7: LGTM: Addition of 'copyloopvar' linterThe addition of the 'copyloopvar' linter is a good replacement for the removed 'exportloopref' linter. This change will help catch potential issues with loop variables being inadvertently shared across iterations, which can lead to subtle bugs. This aligns well with the PR objective of improving code quality and fixing lint errors.
Line range hint
18-20: Approve stricter 'nakedret' settings, but verify impactSetting 'max-func-lines' to 0 for the 'nakedret' linter effectively bans all naked returns in functions. This can improve code readability and reduce potential errors. However, this is a significant change that might affect existing code.
To ensure this change doesn't cause widespread issues, please run the following script to check for potential impacts:
This script will help you identify and quantify the functions that might need to be updated due to this linter change.
✅ Verification successful
The stricter 'nakedret' settings have been verified and no issues were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for naked returns in the codebase that might be affected by the new 'nakedret' setting # Search for functions with naked returns echo "Functions with naked returns:" ast-grep --lang go --pattern 'func $name($params) $return_type { $$$ return }' # Count the occurrences count=$(ast-grep --lang go --pattern 'func $name($params) $return_type { $$$ return }' | wc -l) echo "Total number of functions with naked returns: $count"Length of output: 363
x/blobstream/keeper/keeper_data_commitment.go (1)
112-112: Improved error handling implementationThe change from using
fmt.Sprintfto directly formatting the error message inerrors.Wrapfis a good improvement. It makes the code more concise and addresses the lint error related to non-constant format strings inerrors.Wrapfcalls.
rootulp
added
the
backport:v2.x
|
Added backport to v2.x label b/c I originally noticed these lint issues were on v2.x branch. |
<!-- Please read and fill out this form before submitting your PR. Please make sure you have reviewed our contributors guide before submitting your first PR. --> Closes #3919 ## Overview <!-- Please provide an explanation of the PR, including the appropriate context, background, goal, and rationale. If there is an issue with this information, please provide a tl;dr and link the issue. --> Fix the lint errors of golangci-lint. Merge this [PR](#3923) to this branch. (cherry picked from commit 363a8c5)
Closes #3919 ## Overview Fix the lint errors of golangci-lint. Merge this [PR](#3923) to this branch.<hr>This is an automatic backport of pull request #3924 done by [Mergify](https://mergify.com). --------- Co-authored-by: kobakaku <[email protected]> Co-authored-by: Rootul P <[email protected]>
Closes #3919
Overview
Fix the lint errors of golangci-lint.
Merge this PR to this branch.