Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add shell completion to flag groups #1659

Merged
merged 1 commit into from Jun 21, 2022
Merged

Add shell completion to flag groups #1659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions completions.go
View file
Open in desktop
Expand Up @@ -325,6 +325,9 @@ func (c *Command) getCompletions(args []string) (*Command, []string, ShellCompDi
var completions []string
var directive ShellCompDirective

// Enforce flag groups before doing flag completions
finalCmd.enforceFlagGroupsForCompletion()

// Note that we want to perform flagname completion even if finalCmd.DisableFlagParsing==true;
// doing this allows for completion of persistent flag names even for commands that disable flag parsing.
//
Expand Down
186 changes: 186 additions & 0 deletions completions_test.go
View file
Open in desktop
Expand Up @@ -2691,3 +2691,189 @@ func TestFixedCompletions(t *testing.T) {
t.Errorf("expected: %q, got: %q", expected, output)
}
}

func TestCompletionForGroupedFlags(t *testing.T) {
getCmd := func() *Command {
rootCmd := &Command{
Use: "root",
Run: emptyRun,
}
childCmd := &Command{
Use: "child",
ValidArgsFunction: func(cmd *Command, args []string, toComplete string) ([]string, ShellCompDirective) {
return []string{"subArg"}, ShellCompDirectiveNoFileComp
},
Run: emptyRun,
}
rootCmd.AddCommand(childCmd)

rootCmd.PersistentFlags().Int("ingroup1", -1, "ingroup1")
rootCmd.PersistentFlags().String("ingroup2", "", "ingroup2")

childCmd.Flags().Bool("ingroup3", false, "ingroup3")
childCmd.Flags().Bool("nogroup", false, "nogroup")

// Add flags to a group
childCmd.MarkFlagsRequiredTogether("ingroup1", "ingroup2", "ingroup3")

return rootCmd
}

// Each test case uses a unique command from the function above.
testcases := []struct {
desc string
args []string
expectedOutput string
}{
{
desc: "flags in group not suggested without - prefix",
args: []string{"child", ""},
expectedOutput: strings.Join([]string{
"subArg",
":4",
"Completion ended with directive: ShellCompDirectiveNoFileComp", ""}, "\n"),
},
{
desc: "flags in group suggested with - prefix",
args: []string{"child", "-"},
expectedOutput: strings.Join([]string{
"--ingroup1",
"--ingroup2",
"--ingroup3",
"--nogroup",
":4",
"Completion ended with directive: ShellCompDirectiveNoFileComp", ""}, "\n"),
},
{
desc: "when flag in group present, other flags in group suggested even without - prefix",
args: []string{"child", "--ingroup2", "value", ""},
expectedOutput: strings.Join([]string{
"--ingroup1",
"--ingroup3",
"subArg",
":4",
"Completion ended with directive: ShellCompDirectiveNoFileComp", ""}, "\n"),
},
{
desc: "when all flags in group present, flags not suggested without - prefix",
args: []string{"child", "--ingroup1", "8", "--ingroup2", "value2", "--ingroup3", ""},
expectedOutput: strings.Join([]string{
"subArg",
":4",
"Completion ended with directive: ShellCompDirectiveNoFileComp", ""}, "\n"),
},
{
desc: "group ignored if some flags not applicable",
args: []string{"--ingroup2", "value", ""},
expectedOutput: strings.Join([]string{
"child",
"completion",
"help",
":4",
"Completion ended with directive: ShellCompDirectiveNoFileComp", ""}, "\n"),
},
}

for _, tc := range testcases {
t.Run(tc.desc, func(t *testing.T) {
c := getCmd()
args := []string{ShellCompNoDescRequestCmd}
args = append(args, tc.args...)
output, err := executeCommand(c, args...)
switch {
case err == nil && output != tc.expectedOutput:
t.Errorf("expected: %q, got: %q", tc.expectedOutput, output)
case err != nil:
t.Errorf("Unexpected error %q", err)
}
})
}
}

func TestCompletionForMutuallyExclusiveFlags(t *testing.T) {
getCmd := func() *Command {
rootCmd := &Command{
Use: "root",
Run: emptyRun,
}
childCmd := &Command{
Use: "child",
ValidArgsFunction: func(cmd *Command, args []string, toComplete string) ([]string, ShellCompDirective) {
return []string{"subArg"}, ShellCompDirectiveNoFileComp
},
Run: emptyRun,
}
rootCmd.AddCommand(childCmd)

rootCmd.PersistentFlags().IntSlice("ingroup1", []int{1}, "ingroup1")
rootCmd.PersistentFlags().String("ingroup2", "", "ingroup2")

childCmd.Flags().Bool("ingroup3", false, "ingroup3")
childCmd.Flags().Bool("nogroup", false, "nogroup")

// Add flags to a group
childCmd.MarkFlagsMutuallyExclusive("ingroup1", "ingroup2", "ingroup3")

return rootCmd
}

// Each test case uses a unique command from the function above.
testcases := []struct {
desc string
args []string
expectedOutput string
}{
{
desc: "flags in mutually exclusive group not suggested without the - prefix",
args: []string{"child", ""},
expectedOutput: strings.Join([]string{
"subArg",
":4",
"Completion ended with directive: ShellCompDirectiveNoFileComp", ""}, "\n"),
},
{
desc: "flags in mutually exclusive group suggested with the - prefix",
args: []string{"child", "-"},
expectedOutput: strings.Join([]string{
"--ingroup1",
"--ingroup2",
"--ingroup3",
"--nogroup",
":4",
"Completion ended with directive: ShellCompDirectiveNoFileComp", ""}, "\n"),
},
{
desc: "when flag in mutually exclusive group present, other flags in group not suggested even with the - prefix",
args: []string{"child", "--ingroup1", "8", "-"},
expectedOutput: strings.Join([]string{
"--ingroup1", // Should be suggested again since it is a slice
"--nogroup",
":4",
"Completion ended with directive: ShellCompDirectiveNoFileComp", ""}, "\n"),
},
{
desc: "group ignored if some flags not applicable",
args: []string{"--ingroup1", "8", "-"},
expectedOutput: strings.Join([]string{
"--ingroup1",
"--ingroup2",
":4",
"Completion ended with directive: ShellCompDirectiveNoFileComp", ""}, "\n"),
},
}

for _, tc := range testcases {
t.Run(tc.desc, func(t *testing.T) {
c := getCmd()
args := []string{ShellCompNoDescRequestCmd}
args = append(args, tc.args...)
output, err := executeCommand(c, args...)
switch {
case err == nil && output != tc.expectedOutput:
t.Errorf("expected: %q, got: %q", tc.expectedOutput, output)
case err != nil:
t.Errorf("Unexpected error %q", err)
}
})
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably include a test case for the persistent/local mixing and ensure it behaves on both the parent and subcommand.

On the parent (which is missing the subcommands local flags) the flag groups shouldn't apply at all.

On the subcommand things should behave as normal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

49 changes: 49 additions & 0 deletions flag_groups.go
View file
Open in desktop
Expand Up @@ -172,3 +172,52 @@ func sortedKeys(m map[string]map[string]bool) []string {
sort.Strings(keys)
return keys
}

// enforceFlagGroupsForCompletion will do the following:
// - when a flag in a group is present, other flags in the group will be marked required
// - when a flag in a mutually exclusive group is present, other flags in the group will be marked as hidden
// This allows the standard completion logic to behave appropriately for flag groups
func (c *Command) enforceFlagGroupsForCompletion() {
if c.DisableFlagParsing {
return
}

flags := c.Flags()
groupStatus := map[string]map[string]bool{}
mutuallyExclusiveGroupStatus := map[string]map[string]bool{}
c.Flags().VisitAll(func(pflag *flag.Flag) {
processFlagForGroupAnnotation(flags, pflag, requiredAsGroup, groupStatus)
processFlagForGroupAnnotation(flags, pflag, mutuallyExclusive, mutuallyExclusiveGroupStatus)
})

// If a flag that is part of a group is present, we make all the other flags
// of that group required so that the shell completion suggests them automatically
for flagList, flagnameAndStatus := range groupStatus {
for _, isSet := range flagnameAndStatus {
if isSet {
// One of the flags of the group is set, mark the other ones as required
for _, fName := range strings.Split(flagList, " ") {
_ = c.MarkFlagRequired(fName)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue regarding persistent/local flags mixed; may end up with an error here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, even if there is an error, we can just ignore it since all it will mean is that the non-existent flag couldn't be marked as required.

}
}
}
}

// If a flag that is mutually exclusive to others is present, we hide the other
// flags of that group so the shell completion does not suggest them
for flagList, flagnameAndStatus := range mutuallyExclusiveGroupStatus {
for flagName, isSet := range flagnameAndStatus {
if isSet {
// One of the flags of the mutually exclusive group is set, mark the other ones as hidden
// Don't mark the flag that is already set as hidden because it may be an
// array or slice flag and therefore must continue being suggested
for _, fName := range strings.Split(flagList, " ") {
if fName != flagName {
flag := c.Flags().Lookup(fName)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you'll need to check for nil here due to the same problem you raised on my PR: the case with mixed persistent/local flags and then the command being invoked not having them all.

At definition time we know they'll all exist or panic, but at invocation time we could end up with that case where they were defined properly but not all of them exist on the invoked command.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm safe in this case because thanks to your new hasAllFlags() I will only be getting the list of flags that are all part of the command. But I guess we might as well be safe and add the check, it won't hurt.

flag.Hidden = true
}
}
}
}
}
}