The default ShellCompDirective can be configured

[albers]

Closes #2209.
Is this implementation acceptable?

/cc @thaJeztah

[CLAassistant]

All committers have signed the CLA.

[ccoVeille]

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?

[albers]

Done, PTAL.

[marckhouzam]

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]

[marckhouzam]

Thanks for this @albers.
We need to settle on the API (see in-line comment) and then we'll be good.

[marckhouzam]

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))
```

[albers]

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.

[marckhouzam]

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.

[albers]

@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.

[marckhouzam]

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.

[marckhouzam]

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

[marckhouzam]

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]