-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: add restrictions on the number of args in the CLIs #734
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #734 +/- ##
==========================================
+ Coverage 61.95% 62.00% +0.04%
==========================================
Files 875 875
Lines 98802 98878 +76
==========================================
+ Hits 61217 61307 +90
+ Misses 33983 33962 -21
- Partials 3602 3609 +7
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In x/wasm/client/cli/query.go
, cobra.ExactArgs(0)
is being used.
Is it same with cobra.NoArgs
?
It depends on the meaning of "same". Technically, they are different functions, but both of them throws an error where the number of arguments provided is not empty. |
@@ -94,6 +94,7 @@ $ %s query gov proposal 1 | |||
func GetCmdQueryProposals() *cobra.Command { | |||
cmd := &cobra.Command{ | |||
Use: "proposals", | |||
Args: cobra.NoArgs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why this line is not covered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the current testing structure for the CLIs, codecov cannot detect that the code is covered. So I cannot increase the coverage right now. But, actually, the tests DOES cover the CLIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
This patch will add the restrictions on the number of arguments in the CLIs.
Checklist:
CHANGELOG.md
client/docs/swagger-ui/swagger.yaml