cobra icon indicating copy to clipboard operation
cobra copied to clipboard

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

Open scop opened this issue 3 years ago • 8 comments

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 avatar Jul 10 '21 09:07 scop

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.

scop avatar Jul 10 '21 09:07 scop

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.

marckhouzam avatar Jul 17 '21 00:07 marckhouzam

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

github-actions[bot] avatar Sep 16 '21 00:09 github-actions[bot]

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 avatar Sep 25 '21 00:09 marckhouzam

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: https://github.com/spf13/cobra/blob/4fd30b69ee2b62cf3bbecf0a423f8a1ee47f5f24/completions.go#L203-L212

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?

marckhouzam avatar Sep 27 '21 02:09 marckhouzam

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 avatar Sep 27 '21 18:09 scop

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.

scop avatar Dec 10 '21 18:12 scop

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

marckhouzam avatar Dec 11 '21 20:12 marckhouzam