The default ShellCompDirective can be customized for a command and its subcommands

[albers]

Closes #2209, #2221

#2221 proposed a solution where the custom DefaultShellCompDirective only applied to a given command.

In #2221 (comment), alternative approaches were discussed.
We agreed on a recursive solution, where a change applies to a command and its subcommands.

To ease review, I open a new PR for this approach.

[ccoVeille]

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

[marckhouzam]

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?

[marckhouzam]

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

[albers]

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.

[marckhouzam]

@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

[ccoVeille]

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

[albers]

	childCmd := &Command{
		Use:               "foo",
		CompletionOptions: CompletionOptions{UseParentDefaultShellCompDirective: true,},
	}

This approach does not fulfill the requirements because it is not recursive.
The user would have to define the property on each subcommand that should inherit the setting.

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

The ShellCompDirectives are constants for a good reason.
The should not be modifyable.

• add a function helper

func ShellCompDirectiveHelper(so ShellCompDirective) *ShellCompDirective{
	return &so
}

Well, we could add a ToPointer function for that purpose, WDYT @marckhouzam ?

[marckhouzam]

@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

[albers]

@marckhouzam I updated the documentation.
Maybe we should drop a word about handling of constants in pointers and offer a helper as suggested by @ccoVeille so that the feature can be used either as

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

or

	rootCmd := &Command{
		Use:               "root",
		Run:               emptyRun,
		CompletionOptions: CompletionOptions{DefaultShellCompDirective: ToPointer(ShellCompDirectiveNoFileComp)},
	}

[albers]

@marckhouzam PTAL.

[albers]

ping @marckhouzam

[albers]

@marckhouzam Is there anything I can do to move this PR forward?

[marckhouzam]

Sorry for the delay.
Thanks @albers !