Include shell completion for Cargo#1425
Conversation
Diggsey
left a comment
Diggsey
left a comment
There was a problem hiding this comment.
Thanks for the PR! There are however a few changes needed before I'd be happy accepting it.
| ("completions", Some(c)) => { | ||
| if let Some(shell) = c.value_of("shell") { | ||
| let shell = shell.parse::<Shell>().unwrap(); | ||
| let prefix = "~/.rustup/toolchains/$(rustup toolchain list --default)"; |
There was a problem hiding this comment.
This prefix is not correct - rustup is not necessarily installed here, and the default toolchain isn't necessarily correct.
To get the installation directory of the active toolchain, you should use rustc --print sysroot
There was a problem hiding this comment.
Does to replace the rustup toolchain list --default for rustc --print sysroot solves the problem? I have no way to know the toolchain that the user is using, just the default.
There was a problem hiding this comment.
@Diggsey Help me to finish this, please.
There was a problem hiding this comment.
rustc --print sysroot will call the binary of the currently used toolchain.
| &mut io::stdout(), | ||
| ); | ||
|
|
||
| match shell { |
There was a problem hiding this comment.
Non-trivial logic like this should be pulled out into its own function.
| .setting(AppSettings::DeriveDisplayOrder) | ||
| .setting(AppSettings::SubcommandRequiredElseHelp) | ||
| .subcommand(SubCommand::with_name("list").about("List installed toolchains")) | ||
| .subcommand( |
There was a problem hiding this comment.
This change should not be necessary
There was a problem hiding this comment.
Are you requesting to remove the new --default arg?
| } | ||
|
|
||
| pub fn list_toolchains(cfg: &Cfg) -> Result<()> { | ||
| pub fn list_toolchains(cfg: &Cfg, m: &ArgMatches) -> Result<()> { |
There was a problem hiding this comment.
Please revert the changes to this file as they should not be necessary.
|
☔ The latest upstream changes (presumably #1576) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Closing due to lack of activity and reviewer bandwidth. |
5 participants
rustup completions(Install zsh completions for rustc #387, rustup completions don't include completions for Cargo cargo#5596)rustup toolchain list --default(Machine-readable output #450, rustup show: add flag to show only active toolchain #1406)rustup completions bashinstructions to use home directory (Completion instructions for bash #1199)