cli: add shell completion subcommand #329
Conversation
|
Hmm, I saw error when doing self-checking. But I didn't expect it to be caused by my patch. I will check it. |
liuhangbin
force-pushed
the
shell_dynamic
branch
from
5b182bd
to
f413b91
Compare
There was a problem hiding this comment.
While testing found a few issues or things that might be improved:
- Completion for the
retis collectsub-command does not work. completesub-command is hidden, which is good, but it is used while inferring other sub-commands. This makesretis cfor collection to not work anymore. Ascompleteis a "special" command it would be good to exclude it from that, or maybe the only way is to define a_completesub-command instead.- PR miss changes in
Cargo.lock.
Once the above is done, please add a section in the documentation about how to enable auto-completion (I think it's as simple as adding source <(retis complete --shell bash --register -) in .bashrc.
Thanks!
Welcome to the Rust world :D This is triggered by an update in the linter ( |
I will check this. Looks like I need to register the dynamic cmd after
Currently the Thanks |
No, I think the dynamic completion is quite nice and easier to maintain. Isn't there a way to rename the dynamic completion command or to define it explicitly rather than using the default one? |
From the code it looks like the parameter is hardcode. I have opened an issue for clap, let's see what the maintainer replies. |
Emm, I just found that we can # retis -h
Trace packets on the Linux kernel
Usage: retis [OPTIONS] <COMMAND>
Commands:
collect Collect network events
print Print stored events to stdout
sort Sort stored events in series based on tracking id
pcap Generate a PCAP file from stored events
profile Manage Profiles
Options:
--log-level <LOG_LEVEL> Log level [default: info] [possible values: error, warn, info, debug, trace]
-p, --profile <PROFILE> Comma separated list of profile names to apply
--kconf <KCONF> Path to kernel configuration (e.g. /boot/config-6.3.8-200.fc38.x86_64; default: auto-detect)
-h, --help Print help (see more with '--help')
-V, --version Print version
# source <(retis -g bash)
# retis -g <tab>
bash elvish fish powershell zsh
# retis --generate <tab>
bash elvish fish powershell zsh
# retis p<tab>
print pcap profileIf choose to use this way, should I add the parameter as |
I don't think it absolutely has to be hidden, but to ease the use of Retis the command name should at least not conflict with the other commands (at least
Let me summarize my understanding; we have two choices:
The big difference is the output of 1 needs to be generated for each upgrade in the user's machine to match current commands and options, while 2 queries the current Retis binary and is by definition always up to date. Using From the maintainer of This makes me wonder if dynamic completion would still be a possibility. Looks like it's not advertised (I can understand why) but apart from potential bugs, are there missing functionalities (for Bash for example)?
A dedicated command please. For the name, maybe |
OK
correct
I'm not sure either. There is no document about it...
OK, let me try it first. |
Tried to use subcommand but not work. I even copied the clap_complete example and change the Command::new("completion")
.arg(
Arg::new("generator")
.long("generate")
.value_parser(value_parser!(Shell)),
)
.subcommand(value_hint_command)to let generate = Command::new("generate")
.arg(
Arg::new("shell")
.long("shell")
.value_parser(value_parser!(Shell)),
);
Command::new("completion")
.subcommand(value_hint_command)
.subcommand(generate)It also does not work and reports error |
OK, I read the doc and found that I need to use |
liuhangbin
force-pushed
the
shell_dynamic
branch
from
f413b91
to
2243876
Compare
liuhangbin
changed the title
|
Hi @atenart , recently I use Please tell me if you still want to use BTW, There is no '-c' or '-o' auto-complete for $ retis pcap -h
Generate a PCAP file from stored events
Usage: retis pcap [OPTIONS] --probe <PROBE> [INPUT]
Arguments:
[INPUT] File from which to read events [default: retis.data]
Options:
-p, --probe <PROBE> Filter events from this probe. Probes should follow the [TYPE:]TARGET pattern.
See `retis collect --help` for more details on the probe format.
-o, --out <OUT> Write the generated PCAP output to a file rather than stdio
-h, --help Print help
$ retis collect -h
^CI will check the reason recently. |
There was a problem hiding this comment.
A few nits, but most importantly the completion for the complete command still does not work for me. Only -h and --help are available when hitting <tab> while we should see other parameters (eg. --stack or --filter):
# retis collect -<tab>
-h --help
Please tell me if you still want to use generate-completion as subcommand.
I find generate to be too generic for a command that "only" generates a completion script. The issue with complete would be a conflict with collect for the c use though. If we want to use a sort command name we could go with _complete, but I'm not super sure.
An alternative could be to make something like retis generate completion ....
@vlrpl WDYT?
There is no '-c' or '-o' auto-complete for collect subcommand. But I feel this is the collect parameter's problem.
Agreed. Same with --profile. This might be solved with dynamic completion once they also add support for custom implementation of authorized values.
I think that naming is always hard :)
|
liuhangbin
force-pushed
the
shell_dynamic
branch
from
2243876
to
00736f3
Compare
|
The PR looks mostly good, but still have the below issue.
I had a quick look at this and I'm confident the issue is because of this: impl SubCommand for Collect {
fn thin(&self) -> Result<Command> {
Ok(Command::new("collect").about("Collect network events"))
}
}IMO the correct fix would be to define the same command as what's done in the We should even be able to generate the list of modules and set a |
The first part is a requirement for the The second part, about the auto-completion of collectors for the |
OK, I will see how to fix it.
I'd prefer to fix it on a separate PR. I will put it on my next week's todo list. When 1.4 will be released? |
Thanks!
Target is end of March. |
OK. 2 months should be enough. |
Hi @atenart , I checked this issue. As you said, the diff --git a/src/collect/cli.rs b/src/collect/cli.rs
index 93294883275c..fc348d035f27 100644
--- a/src/collect/cli.rs
+++ b/src/collect/cli.rs
@@ -149,7 +149,7 @@ impl SubCommand for Collect {
}
fn thin(&self) -> Result<Command> {
- Ok(Command::new("collect").about("Collect network events"))
+ self.full()
}
fn name(&self) -> String {Or let all subcommands return full parameter. diff --git a/src/cli/cli.rs b/src/cli/cli.rs
index 459783eeaff1..eb4c1939153b 100644
--- a/src/cli/cli.rs
+++ b/src/cli/cli.rs
@@ -271,9 +271,9 @@ impl ThinCli {
.disable_help_subcommand(true)
.infer_subcommands(true)
.subcommand_required(true);
- // Add thin subcommands so that the main help shows them.
+ // Add full subcommands so that the main help shows them.
for sub in self.subcommands.iter() {
- command = command.subcommand(sub.thin().expect("thin command failed"));
+ command = command.subcommand(sub.full().expect("full command failed"));
}
// Determine the subcommand that was run while ignoring errors from yet-to-be-definedI'd prefer the second way, which should resolve all subcommand auto-completion. What do you think?
For this one, if we could get the possible values during /// Perform full CLI parsing and validation
pub(crate) fn run(mut self) -> Result<CliConfig, ClapError> {
self.enhance_profile().map_err(|err| {
self.command
.error(ErrorKind::InvalidValue, format!("{err}"))
})?;
...
// Replace the ran subcommand with the full subcommand.
self.command = self
.command
.mut_subcommand(self.subcommand.name(), |_| self.subcommand.full().unwrap());Do you have any idea? |
[...]
I agree, this seems to be the best option.
Yes, that does not look straightforward, the cli handling might need some reworking to properly support this. Also not only the I made a quick experiment and this seems to work. But would need some rework (eg. diff --git a/src/cli/cli.rs b/src/cli/cli.rs
index 5a203ea167e0..45ab063a710e 100644
--- a/src/cli/cli.rs
+++ b/src/cli/cli.rs
@@ -100,7 +100,7 @@ pub(crate) trait SubCommand {
/// Generate the clap Command to be used for "full" parsing.
///
/// This method should be called after all dynamic options have been registered.
- fn full(&self) -> Result<Command>;
+ fn full(&mut self) -> Result<Command>;
/// Updates internal structures with clap's ArgMatches.
fn update_from_arg_matches(&mut self, matches: &ArgMatches) -> Result<(), ClapError>;
@@ -165,7 +165,7 @@ where
Ok(<Self as clap::CommandFactory>::command())
}
- fn full(&self) -> Result<Command> {
+ fn full(&mut self) -> Result<Command> {
Ok(<Self as clap::CommandFactory>::command())
}
@@ -272,8 +272,8 @@ impl ThinCli {
.infer_subcommands(true)
.subcommand_required(true);
// Add thin subcommands so that the main help shows them.
- for sub in self.subcommands.iter() {
- command = command.subcommand(sub.thin().expect("thin command failed"));
+ for sub in self.subcommands.iter_mut() {
+ command = command.subcommand(sub.full().expect("thin command failed"));
}
// Determine the subcommand that was run while ignoring errors from yet-to-be-defined
diff --git a/src/collect/cli.rs b/src/collect/cli.rs
index 93294883275c..c0ff1a514811 100644
--- a/src/collect/cli.rs
+++ b/src/collect/cli.rs
@@ -164,7 +164,7 @@ impl SubCommand for Collect {
Some(&mut self.collectors)
}
- fn full(&self) -> Result<Command> {
+ fn full(&mut self) -> Result<Command> {
let long_about = "Collect events using 'collectors'.\n\n \
Collectors are modules that extract \
events from different places of the kernel or userspace daemons \
@@ -173,8 +173,13 @@ impl SubCommand for Collect {
// Determine all registerd collectors and specify both the possible values and the default
// value of the "collectors" argument
- let possible_collectors =
- Vec::from_iter(self.collectors.modules().iter().map(|x| x.to_str()));
+ let mut modules = crate::get_modules()?;
+ let collectors = modules.collectors();
+ let _ = collectors.values().try_for_each(|c| c.register_cli(&mut self.collectors));
+ let possible_collectors: Vec<&'static str> = collectors
+ .keys()
+ .map(|m| m.to_str())
+ .collect();
let full_command = self
.collectors
diff --git a/src/collect/collector.rs b/src/collect/collector.rs
index 0697f4ef0360..1860b1275062 100644
--- a/src/collect/collector.rs
+++ b/src/collect/collector.rs
@@ -473,7 +473,7 @@ impl SubCommandRunner for CollectRunner {
fn run(&mut self, cli: FullCli, modules: Modules) -> Result<()> {
// Initialize collectors.
let mut collectors = Collectors::new(modules)?;
- let cli = collectors.register_cli(cli)?;
+ let cli = cli.run()?;
collectors.init(&cli)?;
collectors.start()?;
// Starts a loop.
diff --git a/src/generate/completion.rs b/src/generate/completion.rs
index 1052ddfa28ed..65063546c533 100644
--- a/src/generate/completion.rs
+++ b/src/generate/completion.rs
@@ -54,7 +54,7 @@ impl SubCommand for Complete {
Ok(<Self as clap::CommandFactory>::command())
}
- fn full(&self) -> Result<Command> {
+ fn full(&mut self) -> Result<Command> {
Ok(<Self as clap::CommandFactory>::command())
}
Also please do not forget this when submitting the final version :) @amorenoz if you have some time, can you check if the above sounds right? |
liuhangbin
force-pushed
the
shell_dynamic
branch
from
00736f3
to
ee280e0
Compare
There was a problem hiding this comment.
Looks good. I'm sorry, one last thing: can you remove fn thin() from impl SubCommand otherwise this PR would "introduce" dead code.
If you can while at it, can you ellaborate a bit more in the doc about auto-completion? Maybe "Retis can generate completion files for shells (Bash, Zsh, Fish...). For example to enable auto-completion of Retis command in Bash, ...".
Thanks!
Add full subcommands for the main help so we can find them when do auto-completion. At the same time remove all thin implementation since it's not used anymore. Update the collect full command about info at the same time. Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
As the clap_complete dynamic module is unstable and hard-codes the
subcommand as `complete`, which conflicts with the current `collect`
subcommand. Let's still use the legacy static way to generate the
completion file.
Usage:
$ retis sh-complete -h
Generate completions file for a specified shell
Usage: retis sh-complete [OPTIONS]
Options:
--shell <SHELL> Specify shell to complete for [possible values: bash, elvish, fish, powershell, zsh]
--register <REGISTER> Path to write completion-registration to
-h, --help Print help
$ retis sh-complete --shell bash --register .
$ source retis.bash
Close: retis-org#68
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
liuhangbin
force-pushed
the
shell_dynamic
branch
from
ee280e0
to
f009cdb
Compare
As the clap_complete dynamic module is unstable and hard-codes the
subcommand as
complete, which conflicts with the currentcollectsubcommand. Let's still use the legacy static way to generate the
completion file.
Usage:
$ retis generate -h
Generate completions file for a specified shell
Usage: retis generate [OPTIONS]
Options:
--shell Specify shell to complete for [possible values: bash, elvish, fish, powershell, zsh]
--register Path to write completion-registration to
-h, --help Print help
$ retis generate --shell bash --register .
$ source retis.bash
Close: #68
v5: Remove
fn thin()fromimpl SubCommand. Update doc description.v4: Update docs/install.md about how to auto-complete. Fix
collectsubcommand auto-completion.v3: Rename subcommand to
sh-complete. Move the generate.rs to new folder generate/completion.rs. Renamestruct Generatetostruct Completeas we rename generate.rs to completion.rs.v2: Use static generator as dynamic generator use hard-code parameter
complete, which conflicts withcollect.