Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ chrono = { version = "0.4", default-features = false, features = ["std"] }
clap = { version = "4", features = ["derive", "wrap_help", "string"] }
clap-cargo = "0.18.3"
clap_complete = "4"
clap_complete_nushell = "4.5"
console = "0.16"
curl = { version = "0.4.44", optional = true }
effective-limits = "0.5.5"
Expand Down
68 changes: 57 additions & 11 deletions src/cli/rustup_mode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ enum RustupSubcmd {
/// Generate tab-completion scripts for your shell
#[command(after_help = completions_help(), arg_required_else_help = true)]
Completions {
shell: Shell,
shell: CompletionShell,

#[arg(default_value = "rustup")]
command: CompletionCommand,
Expand Down Expand Up @@ -1853,8 +1853,57 @@ impl fmt::Display for CompletionCommand {
}
}

#[derive(clap::ValueEnum, Clone, Copy, Debug)]
enum CompletionShell {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems pretty unclear from the code why this needs to be a separate enum rather than another variant added to Shell? Should at least clarify in a comment/docstring.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shell is an external enum so cannot be extended. Do you have some other ideas regarding the extension of such a type? Will nested enums be better in your opinion?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@epage why is this a separate crate? Seems like a pain in the ass for downstreams?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: When I say this PR mirrors the jj one that's because jj has also flattened the enum like this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this a separate crate? Seems like a pain in the ass for downstreams?

There are two distinctions

  • In clap_complete or not
  • In Shell or not

Looking back in the past, I wish we had fewer shells in Shell then we do and in the next major version would like to trim it down and locking it down. imo we shouldn't be implicitly extending the compatibility surface of our callers by adding new content to Shell. Pulling in esoteric shells like elvish and fig should be explicit choices. Where the line is for what should be in clap_complete or even in Shell is not yet determined.

See also

At least with our new completion system, it is easier to add new shells, see https://docs.rs/clap_complete/latest/clap_complete/env/struct.Shells.html

Would be willing to explore something similar for our existing completion system.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe there shouldn't be an enum at all, only a trait?

Bash,
Elvish,
Fish,
Nushell,
PowerShell,
Zsh,
}

impl fmt::Display for CompletionShell {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.write_str(match self {
CompletionShell::Bash => "bash",
CompletionShell::Elvish => "elvish",
CompletionShell::Fish => "fish",
CompletionShell::Nushell => "nushell",
CompletionShell::PowerShell => "powershell",
CompletionShell::Zsh => "zsh",
})
}
}

impl clap_complete::Generator for CompletionShell {
fn file_name(&self, name: &str) -> String {
match self {
CompletionShell::Bash => Shell::Bash.file_name(name),
CompletionShell::Elvish => Shell::Elvish.file_name(name),
CompletionShell::Fish => Shell::Fish.file_name(name),
CompletionShell::PowerShell => Shell::PowerShell.file_name(name),
CompletionShell::Zsh => Shell::Zsh.file_name(name),

CompletionShell::Nushell => clap_complete_nushell::Nushell.file_name(name),
}
}

fn generate(&self, cmd: &clap::Command, buf: &mut dyn Write) {
match self {
CompletionShell::Bash => Shell::Bash.generate(cmd, buf),
CompletionShell::Elvish => Shell::Elvish.generate(cmd, buf),
CompletionShell::Fish => Shell::Fish.generate(cmd, buf),
CompletionShell::PowerShell => Shell::PowerShell.generate(cmd, buf),
CompletionShell::Zsh => Shell::Zsh.generate(cmd, buf),

CompletionShell::Nushell => clap_complete_nushell::Nushell.generate(cmd, buf),
}
}
}

fn output_completion_script(
shell: Shell,
shell: CompletionShell,
command: CompletionCommand,
process: &Process,
) -> Result<ExitCode> {
Expand All @@ -1868,18 +1917,15 @@ fn output_completion_script(
);
}
CompletionCommand::Cargo => {
if let Shell::Zsh = shell {
writeln!(process.stdout().lock(), "#compdef cargo")?;
}

let script = match shell {
Shell::Bash => "/etc/bash_completion.d/cargo",
Shell::Zsh => "/share/zsh/site-functions/_cargo",
CompletionShell::Bash => "/etc/bash_completion.d/cargo",
CompletionShell::Zsh => {
writeln!(process.stdout().lock(), "#compdef cargo")?;
"/share/zsh/site-functions/_cargo"
}
_ => {
return Err(anyhow!(
"{} does not currently support completions for {}",
command,
shell
"{command} does not currently support completions for {shell}",
));
}
};
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading