-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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: Simple support to complete external subcommand #5706
base: master
Are you sure you want to change the base?
Conversation
747dd4c
to
ddcb343
Compare
#[test] | ||
fn suggest_external_subcommand() { | ||
let mut cmd = Command::new("dynamic") | ||
.arg(clap::Arg::new("positional").value_parser(["pos1", "pos2", "pos3"])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a subcommand and adding subcommand candidates when no subcommands are taken is strange. I wonder if we should gate the reading of SubcommandCandidates
based on whether external subcommands are allowed.
#[cfg(feature = "unstable-ext")] | ||
#[allow(clippy::should_implement_trait)] | ||
pub fn add<T: CommandExt + crate::builder::ext::Extension>(mut self, tagged: T) -> Self { | ||
self.app_ext.set(tagged); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app_ext
is different than a command_ext
and we need to carefully consider this.
A Command
can either be a top-level / application command or a subcommand. Command
builder methods deal with one, the other, or both. app_ext
was created for dealing with top-level / application-related data.
Even if we add a new ext
field, we'd need to decide how to name the methods related to it.
#5766 will unblock this |
This PR aims to simply add support for external subcommand completions. It does not provide any support for forward completions regarding issue #5653.