Repository: spf13/cobra
Pull request #1659 (2022-04-09 16:17:28)

Add shell completion to flag groups
This PR teaches the shell completion logic how to handle the new flag group feature of #1654.

To avoid mucking about in the already complex shell completion logic, I took a different approach.
When we are doing shell completion, we first check flag groups like this:
1. when a flag in a group is present, other flags in the group will be marked required
1. when a flag in a mutually exclusive group is present, other flags in the group will be marked as hidden

Since the completion logic already handles required flags and hidden flags, this approach allows the standard completion logic to automatically behave appropriately for flag groups.

To specific changes are in the last commit of this PR.

@johnSchnake @Luap99 

github-actions[bot]: This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.
github-actions[bot]: This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.
johnSchnake: I'll take a look at this on Monday; from me a review on shell completions may be more a learning exercise though so I'd like if someone else could speak to the greater design. Seems like the idea of marking hidden/required is consistent with how the other required flag annotation worked.

Does marking it hidden at this stop not cause any sort of problem if the 'help' has to be printed? e.g. will the hidden flags not be shown in a situation where they should be? Or is it only in the course of autocompletion and out of band from normal functionality?
github-actions[bot]: This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.
github-actions[bot]: This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.
marckhouzam: I meant to answer this earlier but got side-tracked.

> Does marking it hidden at this stop not cause any sort of problem if the 'help' has to be printed? e.g. will the hidden flags not be shown in a situation where they should be? Or is it only in the course of autocompletion and out of band from normal functionality?

Right to the latter.  We are only calling `enforceFlagGroupsForCompletion()` when the `__complete` command is being called and is computing completion choices, so it will not affect any other scenario.

marckhouzam: Ok, this is updated and two new tests have been added as suggested by @johnSchnake.
I've also changed the new tests to be table-based like John did.  It is much nicer to read.
(I hope to eventually move all the completion tests to a table-based format in a separate PR.)
johnSchnake: Code, tests, and approach LGTM but tbh I've never used completions via cobra. @Luap99 , would you mind taking a look and trying out (just since you mentioned it on the originating PR).

Otherwise is fine with me to merge this.
github-actions[bot]: This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.
github-actions[bot]: This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.
marckhouzam: As we are gearing up for the 1.5 release which will contain the eagerly anticipated flag-grouping feature of @johnSchnake, I suggest we merge this PR as well to complete (no pun intended 😆 )  the feature.

@jpmcb are you ok with me merging this?

Patch:
diff --git a/bash_completionsV2.go b/bash_completionsV2.go
index d295791..767bf03 100644
--- a/bash_completionsV2.go
+++ b/bash_completionsV2.go
@@ -219,7 +219,7 @@ __%[1]s_handle_standard_completion_case() {
     local tab=$'\t' comp
 
     # Short circuit to optimize if we don't have descriptions
-    if [[ ${completions[*]} != *$tab* ]]; then
+    if [[ "${completions[*]}" != *$tab* ]]; then
         IFS=$'\n' read -ra COMPREPLY -d '' < <(compgen -W "${completions[*]}" -- "$cur")
         return 0
     fi
diff --git a/command.go b/command.go
index 0f4511f..675bb13 100644
--- a/command.go
+++ b/command.go
@@ -18,6 +18,7 @@ package cobra
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -990,7 +991,7 @@ func (c *Command) ExecuteC() (cmd *Command, err error) {
 	if err != nil {
 		// Always show help if requested, even if SilenceErrors is in
 		// effect
-		if err == flag.ErrHelp {
+		if errors.Is(err, flag.ErrHelp) {
 			cmd.HelpFunc()(cmd, args)
 			return cmd, nil
 		}
@@ -1012,7 +1013,7 @@ func (c *Command) ExecuteC() (cmd *Command, err error) {
 
 func (c *Command) ValidateArgs(args []string) error {
 	if c.Args == nil {
-		return nil
+		return ArbitraryArgs(c, args)
 	}
 	return c.Args(c, args)
 }
diff --git a/completions.go b/completions.go
index 7a9a7fa..2c24839 100644
--- a/completions.go
+++ b/completions.go
@@ -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.
 	//
diff --git a/flag_groups.go b/flag_groups.go
index 1dba424..dc78431 100644
--- a/flag_groups.go
+++ b/flag_groups.go
@@ -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)
+				}
+			}
+		}
+	}
+
+	// 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)
+						flag.Hidden = true
+					}
+				}
+			}
+		}
+	}
+}
diff --git a/user_guide.md b/user_guide.md
index 3a253de..5a7acf8 100644
--- a/user_guide.md
+++ b/user_guide.md
@@ -326,8 +326,8 @@ In both of these cases:
 
 ## Positional and Custom Arguments
 
-Validation of positional arguments can be specified using the `Args` field
-of `Command`.
+Validation of positional arguments can be specified using the `Args` field of `Command`.
+If `Args` is undefined or `nil`, it defaults to `ArbitraryArgs`.
 
 The following validators are built in:
 


Test patch:
diff --git a/command_test.go b/command_test.go
index d48fef1..0446e3c 100644
--- a/command_test.go
+++ b/command_test.go
@@ -1723,6 +1723,38 @@ func TestFlagErrorFunc(t *testing.T) {
 	}
 }
 
+func TestFlagErrorFuncHelp(t *testing.T) {
+	c := &Command{Use: "c", Run: emptyRun}
+	c.PersistentFlags().Bool("help", false, "help for c")
+	c.SetFlagErrorFunc(func(_ *Command, err error) error {
+		return fmt.Errorf("wrap error: %w", err)
+	})
+
+	out, err := executeCommand(c, "--help")
+	if err != nil {
+		t.Errorf("--help should not fail: %v", err)
+	}
+
+	expected := `Usage:
+  c [flags]
+
+Flags:
+      --help   help for c
+`
+	if out != expected {
+		t.Errorf("Expected: %v, got: %v", expected, out)
+	}
+
+	out, err = executeCommand(c, "-h")
+	if err != nil {
+		t.Errorf("-h should not fail: %v", err)
+	}
+
+	if out != expected {
+		t.Errorf("Expected: %v, got: %v", expected, out)
+	}
+}
+
 // TestSortedFlags checks,
 // if cmd.LocalFlags() is unsorted when cmd.Flags().SortFlags set to false.
 // Related to https://github.com/spf13/cobra/issues/404.
diff --git a/completions_test.go b/completions_test.go
index 28a3228..fa087fd 100644
--- a/completions_test.go
+++ b/completions_test.go
@@ -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)
+			}
+		})
+	}
+}
diff --git a/fish_completions_test.go b/fish_completions_test.go
index 37091dc..381c667 100644
--- a/fish_completions_test.go
+++ b/fish_completions_test.go
@@ -3,6 +3,8 @@ package cobra
 import (
 	"bytes"
 	"fmt"
+	"log"
+	"os"
 	"testing"
 )
 
@@ -80,3 +82,57 @@ func TestFishCompletionNoActiveHelp(t *testing.T) {
 	activeHelpVar := activeHelpEnvVar(c.Name())
 	check(t, output, fmt.Sprintf("%s=0", activeHelpVar))
 }
+
+func TestGenFishCompletionFile(t *testing.T) {
+	err := os.Mkdir("./tmp", 0755)
+	if err != nil {
+		log.Fatal(err.Error())
+	}
+
+	defer os.RemoveAll("./tmp")
+
+	rootCmd := &Command{Use: "root", Args: NoArgs, Run: emptyRun}
+	child := &Command{
+		Use:               "child",
+		ValidArgsFunction: validArgsFunc,
+		Run:               emptyRun,
+	}
+	rootCmd.AddCommand(child)
+
+	assertNoErr(t, rootCmd.GenFishCompletionFile("./tmp/test", false))
+}
+
+func TestFailGenFishCompletionFile(t *testing.T) {
+	err := os.Mkdir("./tmp", 0755)
+	if err != nil {
+		log.Fatal(err.Error())
+	}
+
+	defer os.RemoveAll("./tmp")
+
+	f, _ := os.OpenFile("./tmp/test", os.O_CREATE, 0400)
+	defer f.Close()
+
+	rootCmd := &Command{Use: "root", Args: NoArgs, Run: emptyRun}
+	child := &Command{
+		Use:               "child",
+		ValidArgsFunction: validArgsFunc,
+		Run:               emptyRun,
+	}
+	rootCmd.AddCommand(child)
+
+	got := rootCmd.GenFishCompletionFile("./tmp/test", false)
+	if got == nil {
+		t.Error("should raise permission denied error")
+	}
+
+	if os.Getenv("MSYSTEM") == "MINGW64" {
+		if got.Error() != "open ./tmp/test: Access is denied." {
+			t.Errorf("got: %s, want: %s", got.Error(), "open ./tmp/test: Access is denied.")
+		}
+	} else {
+		if got.Error() != "open ./tmp/test: permission denied" {
+			t.Errorf("got: %s, want: %s", got.Error(), "open ./tmp/test: permission denied")
+		}
+	}
+}