-
Notifications
You must be signed in to change notification settings - Fork 135
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
Improve help and positional arg enforcement in most commands #161
Conversation
Chosen approach explained in spf13/cobra#571 (comment) Didn't touch the generic REST commands such as `get`, `delete` etc as I'm confused by them both using `urls.Expand()` to support `get URL` & `get KIND ID` forms, as well as having some Cobra sub-commands.
The |
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.
Just one comment inline not sure if its valid, verified some of the commands locally, looks good 👍
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
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.
This looks very nice, thanks!
One reason why I haven't used the cobra.*Args
functions is because the error messages are not particularly helpful, but now that I see this patch I'm wondering if we could wrap the cobra
functions in such a way that we still defer to them for the validation, but that the error message aligns with the purpose of the command/arg. This could be a later improvement.
Long: "Get description of a role or list of all roles ", | ||
RunE: run, | ||
Long: "Get description of a role or list of all roles", | ||
Args: func(cmd *cobra.Command, args []string) error { |
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.
You could use cobra.MaximumNArgs(1)
here.
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.
For now I figured custom validation lets me have better control of messages.
In this case, MaximumNArgs(1) would print:
Error: accepts at most 1 arg(s), received 2
which is not bad, but I felt saying "at most 1 role name" is more helpful.
I dumped my thoughts on this spf13/cobra#571 (comment)
Part of the problem using built-in validation funcs for us is that we disabled printing usage on error (SilenceUsage: true
).
The default behavior of dumping full description of all args on any error is too annoying for my taste too.
I think best behavior would be to print just the Usage: ocm account roles [flags] [ROLE_NAME]
line on errors (user can always --help for details); if we had that it'd be reasonable to go back to cobra.MaximumNArgs
and friends.
Opened spf13/cobra#1252, currently we can't SetUsageTemplate
to something briefer without breaking --help, working on upstream improvement.
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.
I did add/leave many cobra.*Args and similar, without reviewing each message, largely because of laziness — this PR actually grew from 1 command with wrong help I wanted to fix 🤣
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.
Read your threads upstream and I agree, if we can expose UseLine()
when Arg fails validation that would get us very close to full-lazy cmd definitions :)
@cben yes, I'm comfortable merging this. Thanks for these improvements 👍 /lgtm |
Most commands taking positional arguments didn't document them. Now they do.
Also more consistently using Cobra conventions for
Use:
line, such as "FOO_BAR" rather than "<foo bar>" to describe a single parameter.Chosen approach explained in spf13/cobra#571 (comment)
Behavior change: Many commands silently ignored unexpected positional arguments (e.g.
ocm account status foo
,ocm list clusters mycluster foo
), now they'll print an error.get
,delete
etc.urls.Expand()
to supportget URL
&get KIND ID
forms, but they also have some Cobra sub-commands).