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

feat(completion): add default completion hidden if there are no subcmds #1450

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

scop
Copy link
Contributor

@scop scop commented Jul 10, 2021

This is an alternate/superseding implementation for #1392.

The default completion is useful also in cases where there are no
subcommands, for example to provide completion for flags, and custom
completions for their values.

But when there are no subcommands, make it hidden. Simply adding it
non-hidden would result in a help section to be added just for it which
is just noise: the completion command is not the primary functionality
of the program, yet it would be prominently displayed in help output. It
would also get included among argument completions, which could be a
source of confusion.

@scop
Copy link
Contributor Author

scop commented Jul 10, 2021

Looks like this breaks some tests. Not sure if those indicate bugs in respective tests, as the failing ones get called with the main command name in their args as the first one, as opposed to having all but it there.

But I'm sure we can work those out if the behavior wrt the completion command introduced by this change is seen desirable. Not touching this until there's a conclusion.

@marckhouzam
Copy link
Collaborator

I like this a lot @scop! It keeps the user-experience identical for programs that don't have subcommands and still provides them with completion.

I haven't looked at the test failures, but hopefully they don't indicate an actual bug.

@github-actions
Copy link

This PR is being marked as stale due to a long period of inactivity

@marckhouzam
Copy link
Collaborator

I've looked into the test failures and I believe we have a potential backwards-compatibility issue with this change.

This approach of creating the "completion" command even if there are no other sub-commands means that a program without subcommands will find itself with a subcommand. This seems to have some consequences in behaviour as detected by the tests.

I think it may prevent a program with no subcommands from accepting a generic argument, since Cobra (in legacyArgs()) will give an error because the argument won't match any subcommand (the subcommand being "completion").

This needs more investigation.

@marckhouzam
Copy link
Collaborator

I've looked into this and there is indeed a problem. So, as I started explaining in my previous comment, a program that normally has no sub-commands will accept arguments. But if we add "completion" as a sub-command (the first and only one), then arguments to the program will suddenly be rejected because cobra will instead think the argument should match a valid sub-command.

We had the same problem for the "__complete" command. What was done there is that the "__complete" command is only added if it is currently being called. This allows to call the sub-command, but in any other case, the program thinks it does not have a sub-command.

This is the code that does this:

cobra/completions.go

Lines 203 to 212 in 4fd30b6

c.AddCommand(completeCmd)
subCmd, _, err := c.Find(args)
if err != nil || subCmd.Name() != ShellCompRequestCmd {
// Only create this special command if it is actually being called.
// This reduces possible side-effects of creating such a command;
// for example, having this command would cause problems to a
// cobra program that only consists of the root command, since this
// command would cause the root command to suddenly have a subcommand.
c.RemoveCommand(completeCmd)
}

I've tried doing the same for the "completion" command, in the case there are no other sub-commands, and it seems to work. I've had to fix some issues with the tests also.

@scop do you want to try to code it yourself or shall I try to push an extra commit on your branch?

@scop
Copy link
Contributor Author

scop commented Sep 27, 2021

Oh, please go ahead if you have the time, I don't know when I'll find time to look into this closer again, but it'd be great to see this in!

@scop scop force-pushed the feat/default-completion-when-no-others branch from 53da908 to 0072a58 Compare October 23, 2021 07:24
…mmands

The default completion is useful also in cases where there are no
subcommands, for example to provide completion for flags, and custom
completions for their values.

But when there are no subcommands, make it hidden. Simply adding it
non-hidden would result in a help section to be added just for it which
is just noise: the completion command is not the primary functionality
of the program, yet it would be prominently displayed in help output. It
would also get included among argument completions, which could be a
source of confusion.
@scop scop force-pushed the feat/default-completion-when-no-others branch from 0072a58 to c7cee2a Compare December 10, 2021 18:12
@scop
Copy link
Contributor Author

scop commented Dec 10, 2021

I've done a mechanical rebase of this and #1392 on top of current master. Haven't had a closer look at what @marckhouzam proposed in comments above. If you have the work you did available somewhere still, as a raw patch or a WIP branch, feel free to point me to it. Or as said, you're very welcome to push it to my branch here as well, let me know how you'd like to proceed with this.

@marckhouzam
Copy link
Collaborator

I've posted what I think might be a working solution in #1559 . Please let me know what you think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants