feat: Use structs for CLI-validation errors returned by Cobra. #2266
Conversation
|
|
eth-p
changed the title
errors.go
Outdated
| if e.atMost == -1 && e.atLeast >= 0 { // MinimumNArgs | ||
| return fmt.Sprintf("requires at least %d arg(s), only received %d", e.atLeast, len(e.args)) | ||
| } | ||
|
|
||
| if e.atLeast == -1 && e.atMost >= 0 { // MaximumNArgs | ||
| return fmt.Sprintf("accepts at most %d arg(s), received %d", e.atMost, len(e.args)) | ||
| } | ||
|
|
||
| if e.atLeast == e.atMost && e.atLeast != -1 { // ExactArgs | ||
| return fmt.Sprintf("accepts %d arg(s), received %d", e.atLeast, len(e.args)) | ||
| } | ||
|
|
||
| // RangeArgs | ||
| return fmt.Sprintf("accepts between %d and %d arg(s), received %d", e.atLeast, e.atMost, len(e.args)) | ||
| } |
There was a problem hiding this comment.
I assume -1 is the default value when they are not set.
It's not that obvious.
Maybe code could be clearer by inverting the logic
| if e.atMost == -1 && e.atLeast >= 0 { // MinimumNArgs | |
| return fmt.Sprintf("requires at least %d arg(s), only received %d", e.atLeast, len(e.args)) | |
| } | |
| if e.atLeast == -1 && e.atMost >= 0 { // MaximumNArgs | |
| return fmt.Sprintf("accepts at most %d arg(s), received %d", e.atMost, len(e.args)) | |
| } | |
| if e.atLeast == e.atMost && e.atLeast != -1 { // ExactArgs | |
| return fmt.Sprintf("accepts %d arg(s), received %d", e.atLeast, len(e.args)) | |
| } | |
| // RangeArgs | |
| return fmt.Sprintf("accepts between %d and %d arg(s), received %d", e.atLeast, e.atMost, len(e.args)) | |
| } | |
| if e.atLeast == e.atMost && e.atLeast > 0 { // ExactArgs | |
| return fmt.Sprintf("accepts %d arg(s), received %d", e.atLeast, len(e.args)) | |
| } | |
| if e.atMost >= 0 && e.atLeast >= 0 { // RangeArgs | |
| return fmt.Sprintf("accepts between %d and %d arg(s), received %d", e.atLeast, e.atMost, len(e.args)) | |
| } | |
| if e.atLeast >= 0 { // MinimumNArgs | |
| return fmt.Sprintf("requires at least %d arg(s), only received %d", e.atLeast, len(e.args)) | |
| } | |
| // MaximumNArgs | |
| return fmt.Sprintf("accepts at most %d arg(s), received %d", e.atMost, len(e.args)) | |
| } |
This is pseudo code written on a phone, but I hope you get the idea
There was a problem hiding this comment.
That's much better! Great suggestion, thanks!
errors.go
Outdated
| func (e *UnknownSubcommandError) GetSuggestions() string { | ||
| return e.suggestions | ||
| } |
There was a problem hiding this comment.
Here an for other errors, I would have preferred if this method returned a []string
But it would require to rewrite findSuggestions, or at least to split by \n here, but it would be ugly and dirty
There was a problem hiding this comment.
That's totally fair. I added another commit to split up findSuggestions into two separate functions, and I changed the error structs to hold suggestions []string and return the suggestions slice instead of the merged string.
The `findSuggestions` function keeps the logic for finding the suggestions, and a new `helpTextForSuggestions` function joins them together into a string. This will allow error structs to return the suggestions as a slice, rather than a string.
|
Awesome, thanks for the review @ccoVeille! I rebased the commits against
|
ccoVeille
left a comment
ccoVeille
left a comment
There was a problem hiding this comment.
Please consider these suggestions, and give me feedbacks about them.
I tried to make things easier, and reduce the number of exported struct/constants
errors.go
Outdated
| return fmt.Sprintf("if any flags in the group [%v] are set none of the others can be; %v were all set", e.flagList, e.problemFlags) | ||
| } | ||
|
|
||
| panic("invalid flagGroupType") |
There was a problem hiding this comment.
Didn't you talk about returning an error here?
But maybe, you could take a look at this comment first
There was a problem hiding this comment.
Didn't you talk about returning an error here?
I forgot to push that change after rebase, sorry 😅
| // FlagGroupError is the error returned when mutually-required or | ||
| // mutually-exclusive flags are not properly specified. | ||
| type FlagGroupError struct { | ||
| cmd *Command | ||
| flagList string | ||
| flagGroupType FlagGroupType | ||
| problemFlags []string | ||
| } | ||
|
|
||
| // GetCommand returns the Command that the error occurred in. | ||
| func (e *FlagGroupError) GetCommand() *Command { | ||
| return e.cmd | ||
| } | ||
|
|
||
| // GetFlagList returns the flags in the group. | ||
| func (e *FlagGroupError) GetFlags() []string { | ||
| return strings.Split(e.flagList, " ") | ||
| } | ||
|
|
||
| // GetFlagGroupType returns the type of flag group causing the error. | ||
| // | ||
| // Valid types are: | ||
| // - FlagsAreMutuallyExclusive for mutually-exclusive flags. | ||
| // - FlagsAreRequiredTogether for mutually-required flags. | ||
| // - FlagsAreOneRequired for flags where at least one must be present. | ||
| func (e *FlagGroupError) GetFlagGroupType() FlagGroupType { | ||
| return e.flagGroupType | ||
| } | ||
|
|
||
| // GetProblemFlags returns the flags causing the error. | ||
| // | ||
| // For flag groups where: | ||
| // - FlagsAreMutuallyExclusive, these are all the flags set. | ||
| // - FlagsAreRequiredTogether, these are the missing flags. | ||
| // - FlagsAreOneRequired, this is empty. | ||
| func (e *FlagGroupError) GetProblemFlags() []string { | ||
| return e.problemFlags | ||
| } | ||
|
|
||
| // FlagGroupType identifies which failed validation caused a FlagGroupError. | ||
| type FlagGroupType string | ||
|
|
||
| const ( | ||
| FlagsAreMutuallyExclusive FlagGroupType = "if any is set, none of the others can be" | ||
| FlagsAreRequiredTogether FlagGroupType = "if any is set, they must all be set" | ||
| FlagsAreOneRequired FlagGroupType = "at least one of the flags is required" | ||
| ) | ||
|
|
||
| // Error implements error. | ||
| func (e *FlagGroupError) Error() string { | ||
| switch e.flagGroupType { | ||
| case FlagsAreRequiredTogether: | ||
| return fmt.Sprintf("if any flags in the group [%v] are set they must all be set; missing %v", e.flagList, e.problemFlags) | ||
| case FlagsAreOneRequired: | ||
| return fmt.Sprintf("at least one of the flags in the group [%v] is required", e.flagList) | ||
| case FlagsAreMutuallyExclusive: | ||
| return fmt.Sprintf("if any flags in the group [%v] are set none of the others can be; %v were all set", e.flagList, e.problemFlags) | ||
| } | ||
|
|
||
| panic("invalid flagGroupType") | ||
| } |
There was a problem hiding this comment.
Here is another implementation logic that could help by implementing Unwrap (please read go std errors package for further information)
It avoids to add more exported types,and help with using errors.Is(err,
| // FlagGroupError is the error returned when mutually-required or | |
| // mutually-exclusive flags are not properly specified. | |
| type FlagGroupError struct { | |
| cmd *Command | |
| flagList string | |
| flagGroupType FlagGroupType | |
| problemFlags []string | |
| } | |
| // GetCommand returns the Command that the error occurred in. | |
| func (e *FlagGroupError) GetCommand() *Command { | |
| return e.cmd | |
| } | |
| // GetFlagList returns the flags in the group. | |
| func (e *FlagGroupError) GetFlags() []string { | |
| return strings.Split(e.flagList, " ") | |
| } | |
| // GetFlagGroupType returns the type of flag group causing the error. | |
| // | |
| // Valid types are: | |
| // - FlagsAreMutuallyExclusive for mutually-exclusive flags. | |
| // - FlagsAreRequiredTogether for mutually-required flags. | |
| // - FlagsAreOneRequired for flags where at least one must be present. | |
| func (e *FlagGroupError) GetFlagGroupType() FlagGroupType { | |
| return e.flagGroupType | |
| } | |
| // GetProblemFlags returns the flags causing the error. | |
| // | |
| // For flag groups where: | |
| // - FlagsAreMutuallyExclusive, these are all the flags set. | |
| // - FlagsAreRequiredTogether, these are the missing flags. | |
| // - FlagsAreOneRequired, this is empty. | |
| func (e *FlagGroupError) GetProblemFlags() []string { | |
| return e.problemFlags | |
| } | |
| // FlagGroupType identifies which failed validation caused a FlagGroupError. | |
| type FlagGroupType string | |
| const ( | |
| FlagsAreMutuallyExclusive FlagGroupType = "if any is set, none of the others can be" | |
| FlagsAreRequiredTogether FlagGroupType = "if any is set, they must all be set" | |
| FlagsAreOneRequired FlagGroupType = "at least one of the flags is required" | |
| ) | |
| // Error implements error. | |
| func (e *FlagGroupError) Error() string { | |
| switch e.flagGroupType { | |
| case FlagsAreRequiredTogether: | |
| return fmt.Sprintf("if any flags in the group [%v] are set they must all be set; missing %v", e.flagList, e.problemFlags) | |
| case FlagsAreOneRequired: | |
| return fmt.Sprintf("at least one of the flags in the group [%v] is required", e.flagList) | |
| case FlagsAreMutuallyExclusive: | |
| return fmt.Sprintf("if any flags in the group [%v] are set none of the others can be; %v were all set", e.flagList, e.problemFlags) | |
| } | |
| panic("invalid flagGroupType") | |
| } | |
| // FlagGroupError is the error returned when mutually-required or | |
| // mutually-exclusive flags are not properly specified. | |
| type FlagGroupError struct { | |
| cmd *Command | |
| flagList string | |
| err error | |
| problemFlags []string | |
| } | |
| // GetCommand returns the Command that the error occurred in. | |
| func (e *FlagGroupError) GetCommand() *Command { | |
| return e.cmd | |
| } | |
| // GetFlagList returns the flags in the group. | |
| func (e *FlagGroupError) GetFlags() []string { | |
| return strings.Split(e.flagList, " ") | |
| } | |
| // Unwrap returns the type of flag group causing the error. | |
| // | |
| // Valid types are: | |
| // - ErrFlagsAreMutuallyExclusive for mutually-exclusive flags. | |
| // - ErrFlagsAreRequiredTogether for mutually-required flags. | |
| // - ErrFlagsAreOneRequired for flags where at least one must be present. | |
| func (e *FlagGroupError) Unwrap() error { | |
| return e.err | |
| } | |
| // GetProblemFlags returns the flags causing the error. | |
| // | |
| // For flag groups where: | |
| // - FlagsAreMutuallyExclusive, these are all the flags set. | |
| // - FlagsAreRequiredTogether, these are the missing flags. | |
| // - FlagsAreOneRequired, this is empty. | |
| func (e *FlagGroupError) GetProblemFlags() []string { | |
| return e.problemFlags | |
| } | |
| var ( | |
| ErrFlagsAreMutuallyExclusive = errors.New("if any is set, none of the others can be") | |
| ErrFlagsAreRequiredTogether = errors.New("if any is set, they must all be set") | |
| ErrFlagsAreOneRequired = errors.New("at least one of the flags is required") | |
| ) | |
| // Error implements error. | |
| func (e *FlagGroupError) Error() string { | |
| switch { | |
| case errors.Is(err, ErrFlagsAreRequiredTogether): | |
| return fmt.Sprintf("if any flags in the group [%v] are set they must all be set; missing %v", e.flagList, e.problemFlags) | |
| case errors.Is(e.err, ErrFlagsAreOneRequired): | |
| return fmt.Sprintf("at least one of the flags in the group [%v] is required", e.flagList) | |
| case errors.Is(e.err, ErrFlagsAreMutuallyExclusive): | |
| return fmt.Sprintf("if any flags in the group [%v] are set none of the others can be; %v were all set", e.flagList, e.problemFlags) | |
| } | |
| return e.err | |
| } |
There was a problem hiding this comment.
Good call, yeah. Using errors and errors.Is would be more idiomatic than comparing const strings.
flag_groups.go
Outdated
| return &FlagGroupError{ | ||
| flagList: flagList, | ||
| flagGroupType: FlagsAreRequiredTogether, | ||
| problemFlags: unset, |
There was a problem hiding this comment.
Here and everywhere, I don't see a logic to use a pointer
| return &FlagGroupError{ | |
| flagList: flagList, | |
| flagGroupType: FlagsAreRequiredTogether, | |
| problemFlags: unset, | |
| return FlagGroupError{ | |
| flagList: flagList, | |
| flagGroupType: FlagsAreRequiredTogether, | |
| problemFlags: unset, |
It leads to complicated syntax later, and double pointers so you have to do this
var invalidArgCountErr *cobra.InvalidArgCountError
if errors.As(err, &invalidArgCountErr) {When this could be enough
var invalidArgCountErr cobra.InvalidArgCountError
if errors.As(err, &invalidArgCountErr) {There was a problem hiding this comment.
I totally agree that it makes the syntax complicated. I'm perfectly happy to change it, but I had a reason for originally choosing to use a pointer receiver for the Error() string method: consistency with how the Go standard library implements error structs*.
A couple examples:
- The os/PathError struct, which is used in the example for errors.As.
- The json/SyntaxError struct.
- The exec/ExecError struct.
A newer recent reason we might want to keep the pointer receivers is for consistency with pflag. When I made the PR to add error structs to that project, it ended up being merged with the pointer-receiver error structs.
*I haven't been able to find any official documentation explaining why the standard library chooses to use pointer receivers for errors, but this Reddit comment explains a compile-time type safety benefit of using them over value receivers.
There was a problem hiding this comment.
I know a lot of code in Go standard library are not respecting the Go idiomatic way. So sometimes finding examples in Go code doesn't help much.
Droping @alexandear here.
What do you think about this?
There was a problem hiding this comment.
@eth-p you're completely right about os.PathError and others. But I'm sticking to @ccoVeille's approach of returning a non-pointer FlagGroupError as a simpler option.
Other suggestions:
- Maybe we should make the
FlagGroupErrorstruct unexported (i.e.,flagGroupError)? In the future, we can refactor to return a pointer if needed without breaking compatibility. - Should we return
errorinstead of*FlagGroupError?
There was a problem hiding this comment.
Fair enough. I just pushed the code with the following changes made:
- @ccoVeille's suggestion to make all error structs use value receivers instead of pointer receivers.
- @alexandear's suggestion to have
validateOneRequiredFlagGroups,validateRequiredFlagGroups, andvalidateExclusiveFlagGroupsreturnerrorinstead of*FlagGroupError. I had to add a new parametercmd *Commandso those functions could still create aFlagGroupErrorcontaining the relevantCommand.
Maybe we should make the FlagGroupError struct unexported (i.e., flagGroupError)?
Unfortunately, that would go against the goal for this PR. Adding the error structs is meant to provide a way for callers of Cobra commands to be able to determine the error type and its details without having to parse the error message.
site/content/user_guide.md
Outdated
| var invalidArgCountErr *cobra.InvalidArgCountError | ||
| if errors.As(err, &invalidArgCountErr) { |
There was a problem hiding this comment.
Please read this
flag_groups_test.go
Outdated
| var flagGroupErr *FlagGroupError | ||
| if !errors.As(err, &flagGroupErr) { |
There was a problem hiding this comment.
Please read this
flag_groups.go
Outdated
| } | ||
|
|
||
| func validateExclusiveFlagGroups(data map[string]map[string]bool) error { | ||
| func validateExclusiveFlagGroups(data map[string]map[string]bool) *FlagGroupError { |
There was a problem hiding this comment.
| func validateExclusiveFlagGroups(data map[string]map[string]bool) *FlagGroupError { | |
| func validateExclusiveFlagGroups(data map[string]map[string]bool) FlagGroupError { |
Please read this
flag_groups.go
Outdated
| return &FlagGroupError{ | ||
| flagList: flagList, | ||
| flagGroupType: FlagsAreMutuallyExclusive, | ||
| problemFlags: set, | ||
| } |
There was a problem hiding this comment.
| return &FlagGroupError{ | |
| flagList: flagList, | |
| flagGroupType: FlagsAreMutuallyExclusive, | |
| problemFlags: set, | |
| } | |
| return FlagGroupError{ | |
| flagList: flagList, | |
| flagGroupType: FlagsAreMutuallyExclusive, | |
| problemFlags: set, | |
| } |
Please read this
eth-p
force-pushed
the
error-structs
branch
2 times, most recently
from
5098ed8 to
355d742
Compare
|
I implemented your suggestion to use I wasn't able to create fixup commits without causing merge conflicts when squashing them, so here's another diff to summarize this set of changes: type FlagGroupError struct {
+ err error
cmd *Command
flagList string
- flagGroupType FlagGroupType
problemFlags []string
}
-// FlagGroupType identifies which failed validation caused a FlagGroupError.
-type FlagGroupType string
-
-const (
- FlagsAreMutuallyExclusive FlagGroupType = "if any is set, none of the others can be"
- FlagsAreRequiredTogether FlagGroupType = "if any is set, they must all be set"
- FlagsAreOneRequired FlagGroupType = "at least one of the flags is required"
-)
-
+var (
+ // ErrFlagsAreMutuallyExclusive indicates that more than one flag marked by MarkFlagsMutuallyExclusive was provided.
+ ErrFlagsAreMutuallyExclusive = errors.New("if any is set, none of the others can be")
+
+ // ErrFlagsAreRequiredTogether indicates that only one of the flags marked by MarkFlagsRequiredTogether were provided.
+ ErrFlagsAreRequiredTogether = errors.New("if any is set, they must all be set")
+
+ // ErrFlagsAreOneRequired indicates that none of the flags marked by MarkFlagsOneRequired flags were provided.
+ ErrFlagsAreOneRequired = errors.New("at least one of the flags is required")
+)
+
-// GetFlagGroupType returns the type of flag group causing the error.
-// Valid types are:
-// - FlagsAreMutuallyExclusive for mutually-exclusive flags.
-// - FlagsAreRequiredTogether for mutually-required flags.
-// - FlagsAreOneRequired for flags where at least one must be present.
-func (e *FlagGroupError) GetFlagGroupType() FlagGroupType {
- return e.flagGroupType
-}
+// Unwrap implements errors.Unwrap
+//
+// This returns one of:
+// - ErrFlagsAreMutuallyExclusive
+// - ErrFlagsAreRequiredTogether
+// - ErrFlagsAreOneRequired
+func (e *FlagGroupError) Unwrap() error {
+ return e.err
+}
// Error implements error.
func (e *FlagGroupError) Error() string {
- switch e.flagGroupType {
- case FlagsAreRequiredTogether:
+ switch {
+ case errors.Is(e.err, ErrFlagsAreRequiredTogether):
return fmt.Sprintf("if any flags in the group [%v] are set they must all be set; missing %v", e.flagList, e.problemFlags)
- case FlagsAreOneRequired:
+ case errors.Is(e.err, ErrFlagsAreOneRequired):
return fmt.Sprintf("at least one of the flags in the group [%v] is required", e.flagList)
- case FlagsAreMutuallyExclusive:
+ case errors.Is(e.err, ErrFlagsAreMutuallyExclusive):
return fmt.Sprintf("if any flags in the group [%v] are set none of the others can be; %v were all set", e.flagList, e.problemFlags)
}
- // If the error struct is empty (i.e. wasn't created by Cobra), e.flagGroupType will be an empty string.
- // We don't have a specific message to print, so instead just print the struct contents.
+ // If the error struct is empty (i.e. wasn't created by Cobra), e.err will be nil.
+ // We don't have a message to print, so instead just print the struct contents.
return fmt.Sprintf("%#v", e)
}I left the |
ccoVeille
left a comment
ccoVeille
left a comment
There was a problem hiding this comment.
I like it.
I feel like it still need some minors adjustment, but I would say, let's wait for @marckhouzam feedbacks. He is the maintainer of cobra. He owns the logic of features.
I don't think there is a need to go further before he comes back to you
| func expectErrorAs(err error, target error, t *testing.T) { | ||
| if err == nil { | ||
| t.Fatalf("Expected error, got nil") | ||
| } | ||
|
|
||
| targetType := reflect.TypeOf(target) | ||
| targetPtr := reflect.New(targetType).Interface() // *SomeError | ||
| if !errors.As(err, targetPtr) { | ||
| t.Fatalf("Expected error to be %T, got %T", target, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Why not using simply using this?
| func expectErrorAs(err error, target error, t *testing.T) { | |
| if err == nil { | |
| t.Fatalf("Expected error, got nil") | |
| } | |
| targetType := reflect.TypeOf(target) | |
| targetPtr := reflect.New(targetType).Interface() // *SomeError | |
| if !errors.As(err, targetPtr) { | |
| t.Fatalf("Expected error to be %T, got %T", target, err) | |
| } | |
| } | |
| func expectErrorAs(err error, target error, t *testing.T) { | |
| if err == nil { | |
| t.Fatalf("Expected error, got nil") | |
| } | |
| if !errors.As(err, &target) { | |
| t.Fatalf("Expected error to be %T, got %T", target, err) | |
| } | |
| } |
There was a problem hiding this comment.
Hmm, actually, yeah. Good point. I'll fix it in the next set of changes.
| func getCommandName(c *Command) string { | ||
| if c == nil { | ||
| return "<nil>" | ||
| } else { | ||
| return c.Name() | ||
| } | ||
| } |
There was a problem hiding this comment.
| func getCommandName(c *Command) string { | |
| if c == nil { | |
| return "<nil>" | |
| } else { | |
| return c.Name() | |
| } | |
| } | |
| func getCommandName(c *Command) string { | |
| if c == nil { | |
| return "<nil>" | |
| } | |
| return c.Name() | |
| } |
|
|
||
| err := rootCmd.Execute() | ||
|
|
||
| var invalidArgCountErr cobra.InvalidArgCountError |
There was a problem hiding this comment.
| var invalidArgCountErr cobra.InvalidArgCountError | |
| var errInvalidArgCount cobra.InvalidArgCountError |
|
@marckhouzam could you review this PR ? cc @caarlos0 |
|
This would be really useful in some projects I maintain, especially in Fang. @spf13 @marckhouzam how can I help getting this merged in? :) |
|
@caarlos0 would you be interested in joining the project as a maintainer? Help us evaluate and merge in PRs like this? |
|
@spf13 not sure how much time I can put in, but happy to help as much as I can yes! <3 |
|
Invited. Welcome to the team. |
|
Hey, it's been a while! Did you end up having the time to review this, @caarlos0? |
7 participants
This pull request converts a bunch of
fmt.Errorferrors into structs implementing theerrorinterface.Similar to #2178, the goal of this pull request is to make it easier for callers of a Cobra command to determine why a command failed without having to resort to regex matching or parsing the error message. This pull request goes a bit further, covering all types of CLI validation-related errors (with the exception of
pflag-created ones) and adding methods to get the specific details of the errors.Giving callers this info enables them to do new things such as:
The error structs added are:
InvalidArgCountErrorInvalidArgValueErrorUnknownSubcommandErrorRequiredFlagErrorFlagGroupErrorTests and documentation have been updated as well.