-
-
Notifications
You must be signed in to change notification settings - Fork 140
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(console): add subcommand for gen completions #336
Conversation
tokio-console/src/config.rs
Outdated
/// It should be saved in expected directory, depending on the shell used | ||
/// | ||
/// $ tokio-console gen-completion zsh > $FPATH/_tokio_console |
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.
since we know the shell from the command-line argument, should we just make the command install the completions to the correct location, instead?
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.
@hawkw Maybe it can install the completion but I think that It's better not to do it because the user wants to decide where is put.
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.
hmm, isn't the completion location determined by the shell, rather than the user? i'm fine with the current approach, but think this is worth a potential follow-up.
tokio-console/src/main.rs
Outdated
if let Some(config::OptionalCmd::GenCompletion { shell }) = args.subcmd { | ||
let mut app = config::Config::command(); | ||
generate(shell, &mut app, "tokio-console", &mut std::io::stdout()); | ||
return Ok(()); | ||
} |
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.
style, take it or leave it: now that we have multiple subcommands, we might want to consider collapsing this and the if
block for the GenConfig
subcommand into a match
, like
match args.subcmd {
Some(config::OptionalCmd::GenConfig) => // ...
Some(config::OptionalCmd::GenCompletion { shell } => // ...
None => {},
}
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.
fixed: b51ce10
tokio-console/src/config.rs
Outdated
|
||
/// Generate shell completions | ||
/// | ||
/// It should be saved in expected directory, depending on the shell used |
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.
maybe something like this:
/// It should be saved in expected directory, depending on the shell used | |
/// The completion script should be saved in the shell's completion directory. | |
/// This depends on which shell is in use. | |
/// | |
/// For example, using zsh: |
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.
fixed: 60fbce5
tokio-console/src/config.rs
Outdated
/// It should be saved in expected directory, depending on the shell used | ||
/// | ||
/// $ tokio-console gen-completion zsh > $FPATH/_tokio_console |
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.
hmm, isn't the completion location determined by the shell, rather than the user? i'm fine with the current approach, but think this is worth a potential follow-up.
Ok, I understand your opinion. I'll check how to completion directory the shell that I don't know (fish, elvish, powershell). |
tokio-console/src/config.rs
Outdated
/// | ||
/// For example, using zsh: | ||
/// | ||
/// $ tokio-console gen-completion zsh > ~/.zsh_functions/_tokio_console |
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.
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.
hmm, if this isn't going to work for everyone, maybe we should remove the sample for now, and add it back later if we can come up with one that will work universally?
tokio-console/src/main.rs
Outdated
fn gen_completion(install: bool, shell: Shell) -> color_eyre::Result<()> { | ||
let mut app = config::Config::command(); | ||
let mut buf: Box<dyn std::io::Write> = if install { | ||
let mut home_dir = dirs::home_dir() | ||
.ok_or_else(|| color_eyre::eyre::eyre!("fail to find home directory"))?; | ||
match shell { | ||
Shell::Zsh => { | ||
home_dir.push(".zsh_functions/_tokio_console"); | ||
let f = std::fs::File::create(&home_dir) | ||
.wrap_err_with(|| format!("fail to open {}", home_dir.display()))?; | ||
Box::new(std::io::BufWriter::new(f)) | ||
} | ||
_ => color_eyre::eyre::bail!("Not support to install completion script on {}", shell), | ||
} | ||
} else { | ||
Box::new(std::io::stdout()) | ||
}; | ||
generate(shell, &mut app, "tokio-console", &mut buf); | ||
Ok(()) | ||
} |
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.
@hawkw
How about this approach?
If the user runs as tokio-console gen-completion --install <SHELL>
then put the script file.
But I must fix it, .zsh_functions
directory is not correct. I have a problem.
I have tried to get a directory with $FPATH
environment variables however I failed.
problem:
std::env::var("FPATH")
return an error. I checked on my terminal like this:
❯ echo $FPATH
/home/nrskt/.zsh_functions:....some paths
❯ env | rg FPATH
(return nothing)
So FPATH
not working in my environment. I can't determine if it's a problem with my environment or zsh features.
In my opinion, first I fix it to not support all shells like this:
match shell {
_ => color_eyre::eyre::bail!("Not support to install completion script on {}", shell),
}
And the rest should be treated for potential follow-up.
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.
hmm...it seems like automatically installing completions may be a bit more complicated than i'd hoped. i think it's reasonable to just always fail if --install
is passed, for now, so we can go ahead and merge this branch, and implement support for automatic installations later.
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.
Overall, this looks good! I had some small suggestions.
I think it's fine to save the automatic installation of completion scripts for later, since it looks like it's actually somewhat complex. For now, let's go with what you suggested and have that always fail, and the user can still install completions manually.
tokio-console/src/main.rs
Outdated
@@ -214,3 +223,24 @@ async fn watch_details_stream( | |||
} | |||
} | |||
} | |||
|
|||
fn gen_completion(install: bool, shell: Shell) -> color_eyre::Result<()> { |
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.
nit, take it or leave it: I think it might make sense to put this function in the config
module?
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.
fixed: 99f659f
tokio-console/src/main.rs
Outdated
fn gen_completion(install: bool, shell: Shell) -> color_eyre::Result<()> { | ||
let mut app = config::Config::command(); | ||
let mut buf: Box<dyn std::io::Write> = if install { | ||
let mut home_dir = dirs::home_dir() | ||
.ok_or_else(|| color_eyre::eyre::eyre!("fail to find home directory"))?; | ||
match shell { | ||
Shell::Zsh => { | ||
home_dir.push(".zsh_functions/_tokio_console"); | ||
let f = std::fs::File::create(&home_dir) | ||
.wrap_err_with(|| format!("fail to open {}", home_dir.display()))?; | ||
Box::new(std::io::BufWriter::new(f)) | ||
} | ||
_ => color_eyre::eyre::bail!("Not support to install completion script on {}", shell), | ||
} | ||
} else { | ||
Box::new(std::io::stdout()) | ||
}; | ||
generate(shell, &mut app, "tokio-console", &mut buf); | ||
Ok(()) | ||
} |
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.
hmm...it seems like automatically installing completions may be a bit more complicated than i'd hoped. i think it's reasonable to just always fail if --install
is passed, for now, so we can go ahead and merge this branch, and implement support for automatic installations later.
tokio-console/src/main.rs
Outdated
} else { | ||
Box::new(std::io::stdout()) | ||
}; | ||
generate(shell, &mut app, "tokio-console", &mut buf); |
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.
nit, take it or leave it: i would probably not import the generate
function into this file globally, as it's not super clear what's being generated this way. i would probably have written
generate(shell, &mut app, "tokio-console", &mut buf); | |
clap_complete::generate(shell, &mut app, "tokio-console", &mut buf); |
but, it's not a big deal either way.
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.
fixed: 0c02ebf
tokio-console/src/main.rs
Outdated
.wrap_err_with(|| format!("fail to open {}", home_dir.display()))?; | ||
Box::new(std::io::BufWriter::new(f)) | ||
} | ||
_ => color_eyre::eyre::bail!("Not support to install completion script on {}", shell), |
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.
maybe
_ => color_eyre::eyre::bail!("Not support to install completion script on {}", shell), | |
_ => color_eyre::eyre::bail!("Automatically installing completion scripts is not currently supported on {}", shell), |
tokio-console/src/config.rs
Outdated
/// | ||
/// For example, using zsh: | ||
/// | ||
/// $ tokio-console gen-completion zsh > ~/.zsh_functions/_tokio_console |
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.
hmm, if this isn't going to work for everyone, maybe we should remove the sample for now, and add it back later if we can come up with one that will work universally?
@hawkw Fixed based on your suggestion. How about it? |
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.
okay, this looks good to me! thanks!
#[clap(name = "install", long = "install")] | ||
install: bool, | ||
#[clap(arg_enum)] | ||
shell: Shell, |
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.
i wonder if it's worthwhile to also try to read this from the value of the $SHELL
environment variable, if it's not provided by the user? we could add that in a follow-up.
<a name="0.1.6"></a> ## 0.1.6 (2022-05-23) #### Features * add `Builder::poll_duration_histogram_max` (tokio-rs#351) ([a966feb](a966feb)) #### Bug Fixes * fix memory leak from resizing histograms (tokio-rs#351) ([32dd337](32dd337), closes [tokio-rs#350](350))
This PR includes adding subcommand for shell completions. Generating a man page will be done as another PR.
Related to #325