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 2 commits
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
11 changes: 10 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,13 @@ func (c *Command) getCompletions(args []string) (*Command, []string, ShellCompDi
return finalCmd, completions, directive, nil
}
} else {
directive = ShellCompDirectiveDefault
customShellCompDirective := finalCmd.CompletionOptions.DefaultShellCompDirective
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to settle on what would be the best API for this new option.
Where do we expect the developer to set CompletionOptions.DefaultShellCompDirective?

  1. Do they set it on the root command to set it globally for all the commands and flags of the program? Note that all other completion directives need to be set on the root command
  2. Or should they set it on individual commands and have that option only affect that command and its flags? So, in practice, probably having to set it on many commands.
  3. Set it on a individual commands but have the option recursively affect the command and its flags as well as all its sub-commands and their flags? Here a dev could set the option on the rootCmd and it will affect the entire program, or they could set it only on one or more sub-trees of the command tree.

You've implemented option 2, and I wanted to ask if this was a conscious decision?

Whatever we settle on, I suggest making it clear on the comment above the definition of the new option, and also in the documenation.

Copy link
Author

@albers albers Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@marckhouzam
It was a conscious decision.
Changing the default ShellCompDirective has massive side effects (effectively inverting the logic when and when not to write boilerplate handlers). I prefer an explicit opt-in for this change on the command level that does not propagate to child commands. This allows gradually introducing this new setting in just the places where it makes sense.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You make a good point. But I can't help imagining a developer trying to use this option and being annoyed at having to set it on every single command; some programs have quite a lot of them; I feel like someone will come back and ask for another option to set a "global" default directive 😄 .

If we implement option 3 (affect command and its sub-commands), and a dev only wants to set the option for a single command, they could achieve this by setting the option on the specific command, and then setting the option to ShellCompDirectiveDefault on the first level children of that command. How awkward would that seem to you as an API?

I want to get this as best we can because once it is released, we will not be able to change it (except by introducing another completion option).

@thaJeztah Feel free to give your opinion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ccoVeille If you have an opinion, don't hesitate.
Or anyone else for that matter 😄
This is a community project.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the root command should propagate it, but then:

if someone comes and say "hey, you should set this at root level dude", then some commands may be broken because they expect to have file completion, which is the default one, and it will be broken.

Most CLIs are never tested, first because it's not that easy, then because people are lazy or dislike tests.

So here choosing among the options is more about mitigating the consequences of each of them, and choose the one with the least consequences.

I prefer boilerplate and people having to add a directive on each command than having magical things that will hide problem, because things would be "magical"

Copy link
Author

@albers albers Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

For existing codebases, the second option seems to be most suitable for me because the others are too dangerous, or at least might violate the principle of least surprise.

For new projects, an approch with inheritance is probably a better choice. As a compromise, for those projects I would recommend the third option. It is also more flexible that option one, which assumes that all commands have the same file option density.

The choice depends on how much we weigh each of these use cases.

if customShellCompDirective != ShellCompDirectiveDefault {
directive = customShellCompDirective
} else {
directive = ShellCompDirectiveDefault
}
albers marked this conversation as resolved.
Show resolved Hide resolved

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