fix(client/v2): fix short command description if not set and skip unsupported commands #18324
Conversation
|
Warning Rate Limit Exceeded@julienrbrt has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 13 minutes and 11 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per repository. WalkthroughThe changes introduced in this commit primarily focus on enhancing the functionality of the Changes
TipsChat with CodeRabbit Bot (
|
julienrbrt
added
the
backport/v0.50.x
github-prbot
requested review from
a team,
facundomedica and
atheeshp
and removed request for
a team
|
@julienrbrt your pull request is missing a changelog! |
julienrbrt
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (7)
- client/v2/autocli/common.go (2 hunks)
- client/v2/autocli/msg.go (4 hunks)
- client/v2/autocli/query.go (3 hunks)
- client/v2/autocli/testdata/help-deprecated.golden (1 hunks)
- client/v2/autocli/testdata/help-toplevel-msg.golden (1 hunks)
- client/v2/internal/util/util.go (2 hunks)
- client/v2/internal/util/util_test.go (1 hunks)
Files skipped from review due to trivial changes (2)
- client/v2/autocli/testdata/help-deprecated.golden
- client/v2/internal/util/util_test.go
Additional comments: 12
client/v2/autocli/testdata/help-toplevel-msg.golden (1)
- 5-14: The changes to the available commands look good. Ensure that the new and modified commands ("burn", "multi-send", "set-send-enabled", "update-params") are working as expected and that their respective RPC methods are correctly implemented.
client/v2/autocli/common.go (2)
27-37: The code is providing default values for
shortandlongdescriptions if they are not provided. This is a good practice as it ensures that every command will have a description, improving usability.49-55: The
cobra.Commandstruct is being populated with theshortandlongdescriptions. This is a good practice as it ensures that every command will have a description, improving usability.client/v2/autocli/msg.go (4)
3-9: The import of the "runtime/debug" package is new. Ensure that it is used appropriately and that it does not introduce any potential security or performance issues.
16-16: The import of the "cosmossdk.io/client/v2/internal/util" package is new. Ensure that it is used appropriately and that it does not introduce any potential security or performance issues.
79-85: The
debug.ReadBuildInfo()function is used to get build information. This is a good practice for version checking, but be aware that it can return false if the binary was not built using the go tool.98-100: The
IsSupportedVersionfunction is used to check if a version is supported. This is a good practice for ensuring compatibility.client/v2/autocli/query.go (4)
4-8: The new import "runtime/debug" is used to fetch build information for version compatibility checks. Ensure that this package is compatible with the rest of your codebase.
76-81: The build information is fetched using
debug.ReadBuildInfo(). If it fails to fetch the build information, an emptydebug.BuildInfois created. This is a good practice to avoid nil pointer dereference in the subsequent code.91-100: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [83-96]
The loop iterates over all methods of the service. For each method, it checks if the version is supported using the
util.IsSupportedVersionfunction. If the version is not supported, it skips the current iteration. This is a good practice to ensure compatibility with different versions.
- 98-100: The
BuildQueryMethodCommandfunction is called to build the command for the method. If there is an error, it is returned immediately. This is a good practice for error handling.client/v2/internal/util/util.go (1)
- 1-24: The
DescriptorDocsfunction is marked as not working. If this is still the case, it should be fixed or removed to avoid confusion.- // TODO this does not work, to fix. - func DescriptorDocs(descriptor protoreflect.Descriptor) string { - return descriptor.ParentFile().SourceLocations().ByDescriptor(descriptor).LeadingComments - }
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (7)
- client/v2/autocli/common.go (2 hunks)
- client/v2/autocli/msg.go (4 hunks)
- client/v2/autocli/query.go (3 hunks)
- client/v2/autocli/testdata/help-deprecated.golden (1 hunks)
- client/v2/autocli/testdata/help-toplevel-msg.golden (1 hunks)
- client/v2/internal/util/util.go (2 hunks)
- client/v2/internal/util/util_test.go (1 hunks)
Files skipped from review due to trivial changes (3)
- client/v2/autocli/common.go
- client/v2/autocli/testdata/help-deprecated.golden
- client/v2/internal/util/util_test.go
Additional comments: 10
client/v2/autocli/testdata/help-toplevel-msg.golden (1)
- 5-14: The changes to the available commands look good. The addition of the "burn" command and modifications to the "multi-send", "set-send-enabled", and "update-params" commands align with the summary provided. Ensure that these changes are reflected in the corresponding documentation and user guides.
client/v2/autocli/query.go (3)
4-8: The import of "runtime/debug" is new and is used to get build information for version compatibility checks. Ensure that this package is used correctly and safely.
91-100: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [83-96]
The loop iterates over the methods and checks if the version is supported using
util.IsSupportedVersion(). If not, it skips the method. This is a good practice to ensure compatibility.
- 91-96: The
util.IsSupportedVersion()function is used to check if the version is supported. Ensure that this function is implemented correctly and handles all edge cases.client/v2/autocli/msg.go (5)
3-9: The import of "runtime/debug" is new and is used to read build information for version compatibility checks. Ensure that this package is used responsibly as it can expose sensitive information.
14-16: The import of "cosmossdk.io/client/v2/internal/util" is new and is used for the
IsSupportedVersionfunction. Ensure that this package is imported correctly and that the function is available.76-85: The build information is read and stored in
buildInfo. If the build information cannot be read, an emptyBuildInfostruct is created. This is a good practice to prevent nil pointer dereferences later in the code.98-100: The
IsSupportedVersionfunction is used to check if the version in the method descriptor's comments is supported. If it is not, the loop continues to the next iteration. This is a good practice to prevent unsupported versions from being used.102-104: The
BuildMsgMethodCommandfunction is used to build a command for the method. If there is an error, it is returned immediately. This is a good practice to prevent further execution in the case of an error.client/v2/internal/util/util.go (1)
- 1-24: The
DescriptorDocsfunction is marked as not working. If this is still the case, it should be fixed or removed to avoid confusion.- // TODO this does not work, to fix. - func DescriptorDocs(descriptor protoreflect.Descriptor) string { - return descriptor.ParentFile().SourceLocations().ByDescriptor(descriptor).LeadingComments - }
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (4)
- client/v2/autocli/msg.go (2 hunks)
- client/v2/autocli/query.go (1 hunks)
- client/v2/internal/util/util.go (2 hunks)
- client/v2/internal/util/util_test.go (1 hunks)
Additional comments: 9
client/v2/autocli/msg.go (3)
12-18: The new import statement for the "util" package is correctly placed in alphabetical order.
86-91: The check for supported versions using the "IsSupportedVersion" function is correctly implemented. It correctly skips the loop iteration if the method descriptor is not supported.
if !util.IsSupportedVersion(util.DescriptorDocs(methodDescriptor)) { continue }
- 93-95: Error handling is correctly implemented for the "BuildMsgMethodCommand" function.
methodCmd, err := b.BuildMsgMethodCommand(methodDescriptor, methodOpts) if err != nil { return err }client/v2/internal/util/util.go (6)
1-13: The new imports
"regexp"and"runtime/debug"are added correctly and are used in the code. The global variablebuildInfois also initialized correctly.15-18: The global variable
buildInfois used to store build information. This is a good use of a global variable as it allows the build information to be accessed throughout the package without having to read it multiple times.20-23: The function
DescriptorKebabNameis correctly implemented and returns the kebab case name of a descriptor.36-38: The function
ResolveMessageTypecorrectly resolves a message type from a descriptor.40-44: The function
IsSupportedVersioncorrectly determines if a RPC is supported in the current version.46-68: The function
isSupportedVersioncorrectly checks if a RPC is supported based on the build information. It also correctly handles the case where the input is empty or the build information is nil.
julienrbrt
changed the title
There was a problem hiding this comment.
Nice work and thank you @julienrbrt! LGTM, but just some tightening needed around parsing "// Since"
client/v2/internal/util/util.go
Outdated
| input = strings.ToLower(input) | ||
| input = strings.ReplaceAll(input, "cosmos sdk", "cosmos-sdk") | ||
|
|
||
| re := regexp.MustCompile(`\/\/\s?since: (\S+) (\S+)`) |
There was a problem hiding this comment.
The comment above says "Since", yet this regex uses "since", but we aren't using the case insensitive flag "(?i)" We could simply use "[sS]ince". I highly recommend removing the strings.ToLower then simply using that regexp suggestion that I've made. This way if some module's name is for example PbValidator, it won't be returned as "pbvalidator"
There was a problem hiding this comment.
Case insensitive definitely makes sense, but I am getting that with strings.ToLower.
Additionally, I actually really need to get the module name lowercase to match it properly with the build info.
Lastly, it is easier to replace any writing of CosMoS SdK / Cosmos-SDK -> cosmos-sdk that way.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (2)
- client/v2/internal/util/util.go (2 hunks)
- client/v2/internal/util/util_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- client/v2/internal/util/util_test.go
client/v2/internal/util/util.go
Outdated
| input = strings.ToLower(input) | ||
| input = strings.ReplaceAll(input, "cosmos sdk", "cosmos-sdk") | ||
|
|
||
| re := regexp.MustCompile(`\/\/\s*since: (\S+) (\S+)`) |
There was a problem hiding this comment.
Oh btw I forgot to mention, given that this is a regex from a constant string, let's hoist it out to be a global variable per https://cyber.orijtech.com/scsec/cosmos-go-coding-guide#real-world-exhibit-in-the-cosmos-sdk
There was a problem hiding this comment.
Want to make a PR to https://github.com/cosmos/awesome-cosmos#articles to add this guide?
I forgot about it. Bookmarking now :)
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
New Features
buildMethodCommandCommonfunction if none is provided.AddMsgServiceCommandsfunction to skip unsupported versions.Bug Fixes
Tests
TestIsSupportedVersionandTestParseSinceCommentto validate new utility functions.Refactor
utilpackage with new functions for retrieving descriptor comments and checking supported versions.