Skip to content

completion: improve detection for flags that accept multiple values #2210

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

Merged
merged 2 commits into from
Dec 28, 2024
Merged

completion: improve detection for flags that accept multiple values #2210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
65d3afe
completion: improve detection for flags that accept multiple values
thaJeztah Dec 28, 2024
8c1b26b
Address comments
marckhouzam Dec 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions completions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,14 @@ func (c *Command) initCompleteCmd(args []string) {
}
}

// SliceValue is a reduced version of [pflag.SliceValue]. It is used to detect
// flags that accept multiple values and therefore can provide completion
// multiple times.
type SliceValue interface {
// GetSlice returns the flag value list as an array of strings.
GetSlice() []string
}
Comment on lines +273 to +279
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you do something like this either here or in the tests?

(This is untested pseudocode written on a phone, please check and adapt)

Suggested change
// SliceValue is a reduced version of [pflag.SliceValue]. It is used to detect
// flags that accept multiple values and therefore can provide completion
// multiple times.
type SliceValue interface {
// GetSlice returns the flag value list as an array of strings.
GetSlice() []string
}
// SliceValue is a reduced version of [pflag.SliceValue]. It is used to detect
// flags that accept multiple values and therefore can provide completion
// multiple times.
type SliceValue interface {
// GetSlice returns the flag value list as an array of strings.
GetSlice() []string
}
var _ SliceValue = (*pflag.SliceValue)(nil)

The idea is to validate pflag.SliceValue implements the interface you define

Copy link
Contributor

Choose a reason for hiding this comment

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

@thaJeztah I'm unsure if you saw my comment last week


func (c *Command) getCompletions(args []string) (*Command, []string, ShellCompDirective, error) {
// The last argument, which is not completely typed by the user,
// should not be part of the list of arguments
Expand Down Expand Up @@ -399,10 +407,13 @@ func (c *Command) getCompletions(args []string) (*Command, []string, ShellCompDi
// If we have not found any required flags, only then can we show regular flags
if len(completions) == 0 {
doCompleteFlags := func(flag *pflag.Flag) {
if !flag.Changed ||
_, acceptsMultiple := flag.Value.(SliceValue)
acceptsMultiple = acceptsMultiple ||
strings.Contains(flag.Value.Type(), "Slice") ||
strings.Contains(flag.Value.Type(), "Array") ||
strings.HasPrefix(flag.Value.Type(), "stringTo") {
strings.HasPrefix(flag.Value.Type(), "stringTo")

if !flag.Changed || acceptsMultiple {
// If the flag is not already present, or if it can be specified multiple times (Array, Slice, or stringTo)
// we suggest it as a completion
completions = append(completions, getFlagNameCompletions(flag, toComplete)...)
Expand Down
35 changes: 34 additions & 1 deletion completions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,6 +671,29 @@ func TestFlagNameCompletionInGoWithDesc(t *testing.T) {
}
}

// customMultiString is a custom Value type that accepts multiple values,
// but does not include "Slice" or "Array" in its "Type" string.
type customMultiString []string

var _ SliceValue = (*customMultiString)(nil)

func (s *customMultiString) String() string {
return fmt.Sprintf("%v", *s)
Copy link
Contributor

Choose a reason for hiding this comment

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

What about this?

Suggested change
return fmt.Sprintf("%v", *s)
return fmt.Sprint(*s)

Copy link
Contributor

Choose a reason for hiding this comment

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

Samr here

}

func (s *customMultiString) Set(v string) error {
*s = append(*s, v)
return nil
}

func (s *customMultiString) Type() string {
return "multi string"
}

func (s *customMultiString) GetSlice() []string {
return *s
}

func TestFlagNameCompletionRepeat(t *testing.T) {
rootCmd := &Command{
Use: "root",
Expand All @@ -693,6 +716,8 @@ func TestFlagNameCompletionRepeat(t *testing.T) {
sliceFlag := rootCmd.Flags().Lookup("slice")
rootCmd.Flags().BoolSliceP("bslice", "b", nil, "bool slice flag")
bsliceFlag := rootCmd.Flags().Lookup("bslice")
rootCmd.Flags().VarP(&customMultiString{}, "multi", "m", "multi string flag")
multiFlag := rootCmd.Flags().Lookup("multi")

// Test that flag names are not repeated unless they are an array or slice
output, err := executeCommand(rootCmd, ShellCompNoDescRequestCmd, "--first", "1", "--")
Expand All @@ -706,6 +731,7 @@ func TestFlagNameCompletionRepeat(t *testing.T) {
"--array",
"--bslice",
"--help",
"--multi",
"--second",
"--slice",
":4",
Expand All @@ -728,6 +754,7 @@ func TestFlagNameCompletionRepeat(t *testing.T) {
"--array",
"--bslice",
"--help",
"--multi",
"--slice",
":4",
"Completion ended with directive: ShellCompDirectiveNoFileComp", ""}, "\n")
Expand All @@ -737,20 +764,22 @@ func TestFlagNameCompletionRepeat(t *testing.T) {
}

// Test that flag names are not repeated unless they are an array or slice
output, err = executeCommand(rootCmd, ShellCompNoDescRequestCmd, "--slice", "1", "--slice=2", "--array", "val", "--bslice", "true", "--")
output, err = executeCommand(rootCmd, ShellCompNoDescRequestCmd, "--slice", "1", "--slice=2", "--array", "val", "--bslice", "true", "--multi", "val", "--")
if err != nil {
t.Errorf("Unexpected error: %v", err)
}
// Reset the flag for the next command
sliceFlag.Changed = false
arrayFlag.Changed = false
bsliceFlag.Changed = false
multiFlag.Changed = false

expected = strings.Join([]string{
"--array",
"--bslice",
"--first",
"--help",
"--multi",
"--second",
"--slice",
":4",
Expand All @@ -768,6 +797,7 @@ func TestFlagNameCompletionRepeat(t *testing.T) {
// Reset the flag for the next command
sliceFlag.Changed = false
arrayFlag.Changed = false
multiFlag.Changed = false

expected = strings.Join([]string{
"--array",
Expand All @@ -778,6 +808,8 @@ func TestFlagNameCompletionRepeat(t *testing.T) {
"-f",
"--help",
"-h",
"--multi",
"-m",
"--second",
"-s",
"--slice",
Expand All @@ -797,6 +829,7 @@ func TestFlagNameCompletionRepeat(t *testing.T) {
// Reset the flag for the next command
sliceFlag.Changed = false
arrayFlag.Changed = false
multiFlag.Changed = false

expected = strings.Join([]string{
"-a",
Expand Down
Loading