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.

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 customized for a command and its subcommands #2238

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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: 15 additions & 0 deletions completions.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,13 @@ 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
Comment on lines +118 to +120
Copy link
Contributor

@ccoVeille ccoVeille Feb 18, 2025

Choose a reason for hiding this comment

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

I'm a bit annoyed by something here

We have an exported field with a pointer.

I remember the discussion that occurred in previous PR about that.

But here, the value the user may expect to be able to pass this field when creating an instance of the struct.

But there is a problem, the existing values they may want to pass are constants (with a iota btw)

But then... You cannot pass a reference to a constant.

I mean this cannot be written

_ = cobra.Command{
	DefaultShellCompDirective: &cobra.ShellCompDirectiveNoFileComp
}

So something else need to be found. i think

Either removing the pointer (and use something else like another constant equal to -1), using variables and not constants, adding a setter...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Excellent point @ccoVeille . And I agree that users will try to use the field directly from the structure. That’s why we don’t usually have setters.

@albers I assume you foresaw that and that’s why you created the setter.
I had not thought about it before, but reading the comment from @ccoVeille , I think that instead of using a pointer we could use theshellCompDirectiveMaxValue to indicate the value has not been set

What do you guys think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahhh, my suggestion doesn’t make sense because it’s the compiler that sets the value of DefaultShellCompDirective to 0 for every command. That’s why the pointer works because the compiler sets it to nil.

I need to think about this

Copy link
Author

@albers albers Feb 24, 2025

Choose a reason for hiding this comment

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

In order to support the use case of a child command resetting the DefaultShellCompDirective to ShellCompDirectiveDefault while its parent has a custom DefaultShellCompDirective (see test), we need a way to initialize the value of DefaultShellCompDirective to a new default value like ShellCompDirectiveUndefined or a pointer, while still preserving backwards compatibility.

My first approch may feel a bit clumsy, but it definitely fulfills the requirements. You can create the command with either

	cmd := &Command{Use: "command", Run: emptyRun}
	cmd.CompletionOptions.SetDefaultShellCompDirective(ShellCompDirectiveNoFileComp)

or

	directive := ShellCompDirectiveNoFileComp

	rootCmd := &Command{
		Use:               "root",
		Run:               emptyRun,
		CompletionOptions: CompletionOptions{DefaultShellCompDirective: &directive},
	}

We could improve (?) this approach by making DefaultShellCompDirective private and introduce a getter so that we have a symmetric access pattern that does not tempt users to directly set DefaultShellCompDirective to the constant.

The only alternative I see is to create a factory method newCommand that takes care of initializing DefaultShellCompDirective, something like

    cmd := newCommand(&Command{Use: "command", Run: emptyRun})

But this is definitely not an improvement to the API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@albers your examples have convinced me that this is good enough. Let’s go with it as is, if you can just fix the docs

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get where we are going.

If the argument was "yes, the setter approach was better", I would have say yes. But, here it sounds like something strange to me.

Expecting people to deal with pointer on constants. It looks a bit odd.

here are some suggestions:

  • do not use pointer, but add a completion option boolean that could be used for the parent setting
	childCmd := &Command{
		Use:               "foo",
		CompletionOptions: CompletionOptions{UseParentDefaultShellCompDirective: true,},
	}

I'm unsure this one makes sense, but maybe it would help you guys to find something better around a boolean.

  • keep the pointers, but change the type of ShellComp from constants to variables

people could use

	rootCmd := &Command{
		Use:               "root",
		Run:               emptyRun,
		CompletionOptions: CompletionOptions{DefaultShellCompDirective: &ShellCompDirectiveNoFileComp},
	}
  • add a function helper
func ShellCompDirectiveHelper(so ShellCompDirective) *ShellCompDirective{
	return &so
}
	rootCmd := &Command{
		Use:               "root",
		Run:               emptyRun,
		CompletionOptions: CompletionOptions{DefaultShellCompDirective: ShellCompDirectiveHelper(ShellCompDirectiveNoFileComp)},
	}

}

func (receiver *CompletionOptions) SetDefaultShellCompDirective(directive ShellCompDirective) {
receiver.DefaultShellCompDirective = &directive
Copy link
Collaborator

Choose a reason for hiding this comment

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

if the above suggestion works, we can remove this

}

// Completion is a string that can be used for completions
Expand Down Expand Up @@ -480,6 +487,14 @@ func (c *Command) getCompletions(args []string) (*Command, []Completion, ShellCo
}
} else {
directive = ShellCompDirectiveDefault
// check current and parent commands for a custom DefaultShellCompDirective
for cmd := finalCmd; cmd != nil; cmd = cmd.parent {
if cmd.CompletionOptions.DefaultShellCompDirective != nil {
directive = *cmd.CompletionOptions.DefaultShellCompDirective
break
}
}

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

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

// child1 inherits the custom DefaultShellCompDirective.
childCmd1 := &Command{Use: "child1", Run: emptyRun}
// child2 resets the custom DefaultShellCompDirective to the default value.
childCmd2 := &Command{Use: "child2", Run: emptyRun}
childCmd2.CompletionOptions.SetDefaultShellCompDirective(ShellCompDirectiveDefault)

rootCmd.AddCommand(childCmd1, childCmd2)

testCases := []struct {
desc string
args []string
expectedDirective string
}{
{
"flag completion on root command with custom DefaultShellCompDirective",
[]string{"--string", ""},
"ShellCompDirectiveNoFileComp",
},
{
"flag completion on subcommand with inherited custom DefaultShellCompDirective",
[]string{"child1", "--string", ""},
"ShellCompDirectiveNoFileComp",
},
{
"flag completion on subcommand with reset DefaultShellCompDirective",
[]string{"child2", "--string", ""},
"ShellCompDirectiveDefault",
},
}

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

outputWords := strings.Split(strings.TrimSpace(output), " ")
directive := outputWords[len(outputWords)-1]

if directive != tc.expectedDirective {
t.Errorf("expected: %q, got: %q", tc.expectedDirective, directive)
}
})
}
}
28 changes: 28 additions & 0 deletions site/content/completions/_index.md
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,34 @@ $ helm status --output [tab][tab]
json table yaml
```

#### 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` for a command
Copy link
Collaborator

Choose a reason for hiding this comment

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

“you can change” -> “you can recursively change”

and its subcommands to `ShellCompDirectiveNoFileComp`:

```go
cmd.CompletionOptions.SetDefaultShellCompDirective(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
Collaborator

Choose a reason for hiding this comment

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

Please add:
“To change the default directive for the entire program, set the DefaultShellCompDirective on the root command. "

#### Debugging

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