feat: Use structs for CLI-validation errors returned by Cobra.

[eth-p]

This pull request converts a bunch of fmt.Errorf errors into structs implementing the error interface.

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:

• Printing localized error messages.
• Changing how the errors are displayed (e.g. pretty printing, colors).

The error structs added are:

• InvalidArgCountError
• InvalidArgValueError
• UnknownSubcommandError
• RequiredFlagError
• FlagGroupError

Tests and documentation have been updated as well.

[CLAassistant]

All committers have signed the CLA.

[eth-p]

Awesome, thanks for the review @ccoVeille!

I rebased the commits against main and applied your suggestions. In hindsight it might have been better to keep them as fixup commits for review purposes, but here's a quick TLDR on what changed since your previous review:

• New commit to split up findSuggestions: https://github.com/spf13/cobra/pull/2266/commits/8dd7f633ac91a8fe286b9d6d1ff5d2471eb624b6

• Applied your suggestion to InvalidArgCountError.Error(). I did make a slight change, though:

  - if e.atLeast == e.atMost && e.atLeast > 0 { // ExactArgs
  + if e.atLeast == e.atMost && e.atLeast >= 0 { // ExactArgs
  

  My thoughts were that somebody might do ExactArgs(0), in which case we would want to display the message "accepts 0 arg(s), received N" instead of falling back to the next case and displaying "accepts between 0 and 0 arg(s), received N".

• errors.go:

    type InvalidArgValueError struct {
    	cmd         *Command
    	arg         string
  - 	suggestions string
  + 	suggestions []string
    }
  
  - func (e *InvalidArgValueError) GetSuggestions() string {
  + func (e *InvalidArgValueError) GetSuggestions() []string {
    	return e.suggestions
    }
  
    func (e *InvalidArgValueError) Error() string {
  - 	return fmt.Sprintf("invalid argument %q for %q%s", e.arg, e.cmd.CommandPath(), e.suggestions)
  + 	return fmt.Sprintf("invalid argument %q for %q%s", e.arg, e.cmd.CommandPath(), helpTextForSuggestions(e.suggestions))
    }
  
    type UnknownSubcommandError struct {
    	cmd         *Command
    	subcmd      string
  - 	suggestions string
  + 	suggestions []string
    }
  
  - func (e *UnknownSubcommandError) GetSuggestions() string {
  + func (e *UnknownSubcommandError) GetSuggestions() []string {
    	return e.suggestions
    }
  
    func (e *UnknownSubcommandError) Error() string {
  - 	return fmt.Sprintf("unknown command %q for %q%s", e.subcmd, e.cmd.CommandPath(), e.suggestions)
  + 	return fmt.Sprintf("unknown command %q for %q%s", e.subcmd, e.cmd.CommandPath(), helpTextForSuggestions(e.suggestions))
    }
  
    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)
    	}
  
  + 	// 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.
  + 	return fmt.Sprintf("%#v", e)
  - 	panic("invalid flagGroupType")
    }
  

  I chose to replace the panic in FlagGroupError.Error() as a precaution. I don't think it's likely that someone would create an empty FlagGroupError and call Error() on it, but just in case, I felt like the better approach was to return something instead of causing a panic later down the line when something eventually tried to get the error string.

• errors_test.go:

    func TestInvalidArgValueError_GetSuggestions(t *testing.T) {
  - 	expected := "a"
  + 	expected := []string{"a", "b"}
    	err := &InvalidArgValueError{suggestions: expected}
  
    	got := err.GetSuggestions()
  + 	expectedString := fmt.Sprintf("%#v", expected)
  + 	gotString := fmt.Sprintf("%#v", got)
  - 	if expected != gotString {
  + 	if expectedString != got {
    		t.Fatalf("expected %v, got %v", expected, got)
    	}
    }
  
    func TestUnknownSubcommandError_GetSuggestions(t *testing.T) {
  - 	expected := "a"
  + 	expected := []string{"a", "b"}
    	err := &UnknownSubcommandError{suggestions: expected}
  
    	got := err.GetSuggestions()
  + 	expectedString := fmt.Sprintf("%#v", expected)
  + 	gotString := fmt.Sprintf("%#v", got)
  - 	if expected != gotString {
  + 	if expectedString != got {
    		t.Fatalf("expected %v, got %v", expected, got)
    	}
    }
  

[eth-p]

I implemented your suggestion to use errors.New and errors.Is instead of const strings, and I left a comment explaining why I originally chose to use pointer receivers for the error structs. If you still want me to convert the Error() string functions to value receivers, I can make another set of changes after.

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 fmt.Sprintf("%#v") as is, since err can be nil in the same way flagGroupType could be empty, which would've made return e.err.Error() panic.

[ccoVeille]

@marckhouzam could you review this PR ?

cc @caarlos0

[caarlos0]

This would be really useful in some projects I maintain, especially in Fang.

@spf13 @marckhouzam how can I help getting this merged in? :)

[spf13]

@caarlos0 would you be interested in joining the project as a maintainer? Help us evaluate and merge in PRs like this?

[caarlos0]

@spf13 not sure how much time I can put in, but happy to help as much as I can yes! <3

[spf13]

Invited. Welcome to the team.

[shadiramadan]

Haha @caarlos0 I found this thread through your comment in Fang!

This is great! I was wondering how I could override the usage errors without simply hiding them!

[eth-p]

Hey, it's been a while! Did you end up having the time to review this, @caarlos0?