fix: fixed arg validation for format#198
Conversation
| @@ -56,14 +56,14 @@ func BuildConfigureFormatCommand() *cobra.Command { | |||
| Short: "Sets default format option for output display of AGC commands. Valid format options are 'text' and 'table'", | |||
| Args: cobra.ArbitraryArgs, | |||
There was a problem hiding this comment.
Should this be Args: cobra.ExactArgs(1), to ensure the single argument passed. or is there a case where we would want to have more than one arg passed to this command?
There was a problem hiding this comment.
The feedback from pervious PRs suggested to move away from Args: cobra.ExactArgs(1) to give customers a more readable error when argument is not provided.
There was a problem hiding this comment.
Linking to the discussion on the other thread https://github.com/aws/amazon-genomics-cli/pull/157/files/d6efecb9dcbba2d8736b81db8c9292f507e336e4#r745907823
i can take an argument on better error message, for Error: accepts 1 arg(s), received 0 vs a single format value must be provided does the latter really looks better? If we're going to the extent of avoiding cobra provided logic and building our own here could we give a better message then?
with us having two allowed values for the command i think we could do something like
Args: cobra.OnlyValidArgs,
ValidArgs: []string{"text", "table"},
or something like that could be an option too, potentially allowing us to use the values in autocomplete too?
There was a problem hiding this comment.
"Error: accepts 1 arg(s), received 0" only tells the customer the what while "Error: a single format value must be provided" tells the customer the why as well. This means that for a customer to resolve the issue, they don't need to type "-h" to know what the values are.
Ideally, we could populated the valid args like you mention or generate the message with the valid options like we do with some of our other commands.
|
|
||
| func (o *formatContextOpts) Validate() error { | ||
| if o.formatContextVars.format == "" { | ||
| func (o *formatContextOpts) Validate(args []string) error { |
There was a problem hiding this comment.
Should we be passing an array in here if we're only reading args[0] out of it later on?
There was a problem hiding this comment.
That function is handling validation which also checks if the array is empty to provide users with a readable error message
| func (o *formatContextOpts) Validate() error { | ||
| if o.formatContextVars.format == "" { | ||
| func (o *formatContextOpts) Validate(args []string) error { | ||
| if len(args) == 0 { |
There was a problem hiding this comment.
This should be != 1 with a message similar to "a single format value must be provided"
3 participants
Issue #, if available:
Description of Changes
Argument checks for
agc configure formatwas not handled gracefully. This caused stack error to output into the console.Description of how you validated changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license