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

The default ShellCompDirective can be configured #2221

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

The default ShellCompDirective can be configured #2221

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
440c33b
The default ShellCompDirective can be configured
albers Jan 19, 2025
33e3ac1
Be more explicit about the default ShellCompDirective
albers Jan 20, 2025
b4f26ae
Apply suggestions from code review
albers Jan 21, 2025
5df541d
Resolve gofmt error
albers Jan 26, 2025
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
5 changes: 4 additions & 1 deletion completions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,9 @@ type CompletionOptions struct {
DisableDescriptions bool
// HiddenDefaultCmd makes the default 'completion' command hidden
HiddenDefaultCmd bool
// DefaultShellCompDirective sets the ShellCompDirective that is returned
// if no special directive can be determined
DefaultShellCompDirective ShellCompDirective
}

// NoFileCompletions can be used to disable file completion for commands that should
Expand Down Expand Up @@ -451,7 +454,7 @@ func (c *Command) getCompletions(args []string) (*Command, []string, ShellCompDi
return finalCmd, completions, directive, nil
}
} else {
directive = ShellCompDirectiveDefault
directive = finalCmd.CompletionOptions.DefaultShellCompDirective
Copy link
Contributor

Choose a reason for hiding this comment

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

This code then rely on the fact the default value of this variable is 0 and then matches ShellCompDirectiveDefault

It's a bit tricky and invisible, I'm not fan of this.

Could you make it more explicit?

Copy link
Author

Choose a reason for hiding this comment

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

Done, PTAL.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a bit tricky and invisible, I'm not fan of this.

Agreed but the explicit code just feels "wrong" to me. I mean:

directive = ShellCompDirectiveDefault

customShellCompDirective := finalCmd.CompletionOptions.DefaultShellCompDirective
if customShellCompDirective != ShellCompDirectiveDefault {
	directive = customShellCompDirective
}

when I read the if statement, I immediately wonder why we have this effectively non-useful if statement.
Furthermore, if we were to change the first line and use a different directive, it would require to change the if statement, which seems unnecessarily brittle.
Finally, looking at the if statement, one still wonders what happens when the new option is not set by the program and will need to realize it is set by the compiler.

How going back to the original one-line but adding a comment above it:
// If not explicitly set by the program, the default directive will be [ShellCompDirectiveDefault]

if flag == nil {
foundLocalNonPersistentFlag := false
// If TraverseChildren is true on the root command we don't check for
Expand Down
42 changes: 42 additions & 0 deletions completions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3742,3 +3742,45 @@ func TestDisableDescriptions(t *testing.T) {
})
}
}

func TestCustomDefaultShellCompDirective(t *testing.T) {
rootCmd := &Command{Use: "root", Run: emptyRun}
rootCmd.Flags().String("string", "", "test string flag")
// use ShellCompDirectiveNoFileComp instead of the default, which is ShellCompDirectiveDefault
rootCmd.CompletionOptions.DefaultShellCompDirective = ShellCompDirectiveNoFileComp

testCases := []struct {
desc string
args []string
}{
{
"args completion with custom ShellCompDirective",
[]string{""},
},
{
"flag completion with custom ShellCompDirective",
[]string{"--string", ""},
},
}

for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
args := []string{ShellCompNoDescRequestCmd}
args = append(args, tc.args...)

output, err := executeCommand(rootCmd, args...)

if err != nil {
t.Errorf("Unexpected error: %v", err)
}

expected := strings.Join([]string{
":4",
"Completion ended with directive: ShellCompDirectiveNoFileComp", ""}, "\n")

if output != expected {
t.Errorf("expected: %q, got: %q", expected, output)
}
})
}
}
23 changes: 23 additions & 0 deletions site/content/completions/_index.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,29 @@ $ helm status --output [tab][tab]
json table yaml
```

#### Change the default ShellCompDirective

If Cobra cannot determine a special `ShellCompDirective` during flag parsing,
it will return `ShellCompDirectiveDefault`, which will invoke the shell's filename completion.
This is handy for flags that accept filenames, as they do not require a `FlagCompletionFunc`.

For other flags where no meaningful completion can be provided, this requires extra work:
You have to register a `FlagCompletionFunc` just to get rid of file completion.

If you find yourself registering lots of handlers like

```go
cmd.RegisterFlagCompletionFunc("flag-name", cobra.NoFileCompletions)
```

you can change the default `ShellCompDirective` to `ShellCompDirectiveNoFileComp`:

```go
cmd.CompletionOptions.DefaultShellCompDirective = ShellCompDirectiveNoFileComp
```

Keep in mind that from now on you have to register handlers for every filename flag.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What do think of the slightly reworked:

#### Change the default ShellCompDirective

When no completion function is registered for a leaf command or for a flag, Cobra will
automatically use `ShellCompDirectiveDefault`, which will invoke the shell's filename completion.
This implies that when file completion does not apply to a leaf command or to a flag (the command
or flag does not operate on a filename), turning off file completion requires you to register a
completion function for that command/flag.  For example:

```go
cmd.RegisterFlagCompletionFunc("flag-name", cobra.NoFileCompletions)
```

If you find that there are more situations where file completion should be turned off than
when it is applicable, you can change the default `ShellCompDirective` to `ShellCompDirectiveNoFileComp`:

```go
rootCmd.CompletionOptions.DefaultShellCompDirective = ShellCompDirectiveNoFileComp
```

If doing so, keep in mind that you should instead register a completion function for leaf commands or
flags where file completion is applicable. For example:

```go
cmd.RegisterFlagCompletionFunc("flag-name", cobra.FixedCompletions(nil, ShellCompDirectiveDefault))
```

Copy link
Author

Choose a reason for hiding this comment

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

This is much better than my version, thank you very much.
I'll incorporate it as soon as the API discussion is settled, because depending on its outcome, rootCmd might be changed to cmd in the second code block.

#### Debugging

You can also easily debug your Go completion code for flags:
Expand Down
Loading