Skip to content

Commit

Permalink
Only return first error and process in sorted order
Browse files Browse the repository at this point in the history
  • Loading branch information
@johnSchnake
johnSchnake committed Apr 9, 2022
1 parent e90250d commit ea529ed
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 45 deletions.
65 changes: 30 additions & 35 deletions flag_groups.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ func (c *Command) MarkFlagsMutuallyExclusive(flagNames ...string) {
}
}

// validateFlagGroups validates the mutuallyExclusive/requiredAsGroup logic.
// validateFlagGroups validates the mutuallyExclusive/requiredAsGroup logic and returns the
// first error encountered.
func (c *Command) validateFlagGroups() error {
if c.DisableFlagParsing {
return nil
Expand All @@ -70,12 +71,11 @@ func (c *Command) validateFlagGroups() error {
processFlagForGroupAnnotation(pflag, mutuallyExclusive, mutuallyExclusiveGroupStatus)
})

errs := validateRequiredFlagGroups(groupStatus)
errsExclusive := validateExclusiveFlagGroups(mutuallyExclusiveGroupStatus)
errs = append(errs, errsExclusive...)

if len(errs) > 0 {
return combineErrors(errs, "\n")
if err := validateRequiredFlagGroups(groupStatus); err != nil {
return err
}
if err := validateExclusiveFlagGroups(mutuallyExclusiveGroupStatus); err != nil {
return err
}
return nil
}
Expand All @@ -98,9 +98,11 @@ func processFlagForGroupAnnotation(pflag *flag.Flag, annotation string, groupSta
}
}

func validateRequiredFlagGroups(data map[string]map[string]bool) []error {
var errs []error
for flagList, flagnameAndStatus := range data {
func validateRequiredFlagGroups(data map[string]map[string]bool) error {
keys := sortedKeys(data)
for _, flagList := range keys{
flagnameAndStatus := data[flagList]

unset := []string{}
for flagname, isSet := range flagnameAndStatus {
if !isSet {
Expand All @@ -113,14 +115,16 @@ func validateRequiredFlagGroups(data map[string]map[string]bool) []error {

// Sort values, so they can be tested/scripted against consistently.
sort.Strings(unset)
errs = append(errs, fmt.Errorf("if any flags in the group [%v] are set they must all be set; missing %v", flagList, unset))
return fmt.Errorf("if any flags in the group [%v] are set they must all be set; missing %v", flagList, unset)
}
return errs

return nil
}

func validateExclusiveFlagGroups(data map[string]map[string]bool) []error {
var errs []error
for flagList, flagnameAndStatus := range data {
func validateExclusiveFlagGroups(data map[string]map[string]bool) error {
keys := sortedKeys(data)
for _, flagList := range keys{
flagnameAndStatus := data[flagList]
var set []string
for flagname, isSet := range flagnameAndStatus {
if isSet {
Expand All @@ -133,27 +137,18 @@ func validateExclusiveFlagGroups(data map[string]map[string]bool) []error {

// Sort values, so they can be tested/scripted against consistently.
sort.Strings(set)
errs = append(errs, fmt.Errorf("if any flags in the group [%v] are set none of the others can be; %v were all set", flagList, set))
return fmt.Errorf("if any flags in the group [%v] are set none of the others can be; %v were all set", flagList, set)
}
return errs
return nil
}

// combineErrors will squash the errors together into a single error with the messages
// joined as a bulleted list.
func combineErrors(errs []error, sep string) error {
if len(errs) == 0 {
return nil
}
if len(errs) == 1{
return errs[0]
}

var msgs []string
for _, e := range errs {
msgs = append(msgs, "- " + e.Error())
func sortedKeys(m map[string]map[string]bool) ([]string) {
keys := make([]string, len(m))
i := 0
for k := range m {
keys[i] = k
i++
}

// Sort values, so they can be tested/scripted against consistently.
sort.Strings(msgs)
return fmt.Errorf(strings.Join(msgs, sep))
}
sort.Strings(keys)
return keys
}
32 changes: 22 additions & 10 deletions flag_groups_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,25 +59,37 @@ func TestValidateFlagGroups(t *testing.T) {
args: []string{"--a=foo", "--b=foo"},
expectErr: "if any flags in the group [a b c] are set none of the others can be; [a b] were all set",
}, {
desc: "Multiple required flag group not satisfied",
desc: "Multiple required flag group not satisfied returns first error",
flagGroupsRequired: []string{"a b c", "a d"},
args: []string{"--c=foo", "--d=foo"},
expectErr: `- if any flags in the group [a b c] are set they must all be set; missing [a b]
- if any flags in the group [a d] are set they must all be set; missing [a]`,
expectErr: `if any flags in the group [a b c] are set they must all be set; missing [a b]`,
}, {
desc: "Multiple exclusive flag group not satisfied",
desc: "Multiple exclusive flag group not satisfied returns first error",
flagGroupsExclusive: []string{"a b c", "a d"},
args: []string{"testcmd", "--a=foo", "--c=foo", "--d=foo"},
expectErr: `- if any flags in the group [a b c] are set none of the others can be; [a c] were all set
- if any flags in the group [a d] are set none of the others can be; [a d] were all set`,
expectErr: `if any flags in the group [a b c] are set none of the others can be; [a c] were all set`,
}, {
desc: "Persistent flags utilize both features and can fail",
desc: "Validation of required groups occurs on groups in sorted order",
flagGroupsRequired: []string{"a d", "a b", "a c"},
args: []string{"testcmd", "--a=foo"},
expectErr: `if any flags in the group [a b] are set they must all be set; missing [b]`,
},{
desc: "Validation of exclusive groups occurs on groups in sorted order",
flagGroupsExclusive: []string{"a d", "a b", "a c"},
args: []string{"testcmd", "--a=foo", "--b=foo", "--c=foo"},
expectErr: `if any flags in the group [a b] are set none of the others can be; [a b] were all set`,
},{
desc: "Persistent flags utilize both features and can fail required groups",
flagGroupsRequired: []string{"a e", "e f"},
flagGroupsExclusive: []string{"f g"},
args: []string{"testcmd", "--a=foo", "--f=foo", "--g=foo"},
expectErr: `- if any flags in the group [a e] are set they must all be set; missing [e]
- if any flags in the group [e f] are set they must all be set; missing [e]
- if any flags in the group [f g] are set none of the others can be; [f g] were all set`,
expectErr: `if any flags in the group [a e] are set they must all be set; missing [e]`,
}, {
desc: "Persistent flags utilize both features and can fail mutually exclusive groups",
flagGroupsRequired: []string{"a e", "e f"},
flagGroupsExclusive: []string{"f g"},
args: []string{"testcmd", "--a=foo", "--e=foo","--f=foo", "--g=foo"},
expectErr: `if any flags in the group [f g] are set none of the others can be; [f g] were all set`,
}, {
desc: "Persistent flags utilize both features and can pass",
flagGroupsRequired: []string{"a e", "e f"},
Expand Down

0 comments on commit ea529ed

Please sign in to comment.