Skip to content
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

Avoid new deprecation warnings from clap 3.1.0 #10396

Merged
merged 4 commits into from
Feb 17, 2022
Merged
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
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ toml_edit = { version = "0.13.4", features = ["serde", "easy"] }
unicode-xid = "0.2.0"
url = "2.2.2"
walkdir = "2.2"
clap = "3.0.13"
clap = "3.1.0"
unicode-width = "0.1.5"
openssl = { version = '0.10.11', optional = true }
im-rc = "15.0.0"
Expand Down
30 changes: 18 additions & 12 deletions src/bin/cargo/cli.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
use anyhow::anyhow;
use cargo::core::{features, CliUnstable};
use cargo::{self, drop_print, drop_println, CliResult, Config};
use clap::{AppSettings, Arg, ArgMatches};
use clap::{
error::{ContextKind, ContextValue},
AppSettings, Arg, ArgMatches,
};
use itertools::Itertools;
use std::collections::HashMap;
use std::fmt::Write;
Expand Down Expand Up @@ -33,9 +36,17 @@ pub fn main(config: &mut Config) -> CliResult {
let args = match cli().try_get_matches() {
Ok(args) => args,
Err(e) => {
if e.kind == clap::ErrorKind::UnrecognizedSubcommand {
if e.kind() == clap::ErrorKind::UnrecognizedSubcommand {
// An unrecognized subcommand might be an external subcommand.
let cmd = e.info[0].clone();
let cmd = e
.context()
.find_map(|c| match c {
(ContextKind::InvalidSubcommand, &ContextValue::String(ref cmd)) => {
Some(cmd)
}
_ => None,
})
Comment on lines +41 to +48
Copy link
Member

@dtolnay dtolnay Feb 17, 2022

Choose a reason for hiding this comment

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

Clap feature request: this would be better as:

let cmd = e.context::<clap::context::InvalidSubcommand>().expect(...);

where the signature is:

impl Error {
    fn context<Kind: ContextKind>(&self) -> Option<&Kind::ContextValue>;
}

It seems clumsy that to access the piece of context you want requires iterating arbitrary other context kinds and having to assert that clap didn't stick your context in there with a type that it wasn't supposed to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this code made me really sad to write.

The proposed interface may have to be able to deal with multiple values for a given kind, I'm not sure, but worth thinking about for sure 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I started with the most general API, awaiting seeing how it was going to be used to know what would be helpful.

Since fn context() is already in use, we'll just have to come up with a different name for this.

.expect("UnrecognizedSubcommand implies the presence of InvalidSubcommand");
return super::execute_external_subcommand(config, &cmd, &[&cmd, "--help"])
.map_err(|_| e.into());
} else {
Expand Down Expand Up @@ -286,9 +297,7 @@ For more information, see issue #10049 <https://github.com/rust-lang/cargo/issue
// Note that an alias to an external command will not receive
// these arguments. That may be confusing, but such is life.
let global_args = GlobalArgs::new(args);
let new_args = cli()
.setting(AppSettings::NoBinaryName)
.try_get_matches_from(alias)?;
let new_args = cli().no_binary_name(true).try_get_matches_from(alias)?;

let new_cmd = new_args.subcommand_name().expect("subcommand is required");
already_expanded.push(cmd.to_string());
Expand Down Expand Up @@ -406,14 +415,11 @@ fn cli() -> App {
"cargo [OPTIONS] [SUBCOMMAND]"
};
App::new("cargo")
.setting(
AppSettings::DeriveDisplayOrder
| AppSettings::AllowExternalSubcommands
| AppSettings::NoAutoVersion,
)
.allow_external_subcommands(true)
.setting(AppSettings::DeriveDisplayOrder | AppSettings::NoAutoVersion)
// Doesn't mix well with our list of common cargo commands. See clap-rs/clap#3108 for
// opening clap up to allow us to style our help template
.global_setting(AppSettings::DisableColoredHelp)
.disable_colored_help(true)
jonhoo marked this conversation as resolved.
Show resolved Hide resolved
.override_usage(usage)
.help_template(
"\
Expand Down
2 changes: 1 addition & 1 deletion src/bin/cargo/commands/bench.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use cargo::ops::{self, TestOptions};

pub fn cli() -> App {
subcommand("bench")
.setting(AppSettings::TrailingVarArg)
.trailing_var_arg(true)
.about("Execute all benchmarks of a local package")
.arg_quiet()
.arg(
Expand Down
3 changes: 2 additions & 1 deletion src/bin/cargo/commands/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ pub fn cli() -> App {
subcommand("config")
.about("Inspect configuration values")
.after_help("Run `cargo help config` for more detailed information.\n")
.setting(clap::AppSettings::SubcommandRequiredElseHelp)
.subcommand_required(true)
jonhoo marked this conversation as resolved.
Show resolved Hide resolved
.arg_required_else_help(true)
.subcommand(
subcommand("get")
.arg(Arg::new("key").help("The config key to display"))
Expand Down
2 changes: 1 addition & 1 deletion src/bin/cargo/commands/git_checkout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const REMOVED: &str = "The `git-checkout` subcommand has been removed.";
pub fn cli() -> App {
subcommand("git-checkout")
.about("This subcommand has been removed")
.setting(AppSettings::Hidden)
.hide(true)
.override_help(REMOVED)
}

Expand Down
3 changes: 2 additions & 1 deletion src/bin/cargo/commands/report.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ pub fn cli() -> App {
subcommand("report")
.about("Generate and display various kinds of reports")
.after_help("Run `cargo help report` for more detailed information.\n")
.setting(clap::AppSettings::SubcommandRequiredElseHelp)
.subcommand_required(true)
jonhoo marked this conversation as resolved.
Show resolved Hide resolved
.arg_required_else_help(true)
.subcommand(
subcommand("future-incompatibilities")
.alias("future-incompat")
Expand Down
2 changes: 1 addition & 1 deletion src/bin/cargo/commands/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ pub fn cli() -> App {
subcommand("run")
// subcommand aliases are handled in aliased_command()
// .alias("r")
.setting(AppSettings::TrailingVarArg)
.trailing_var_arg(true)
.about("Run a binary or example of the local package")
.arg_quiet()
.arg(
Expand Down
2 changes: 1 addition & 1 deletion src/bin/cargo/commands/rustc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ const CRATE_TYPE_ARG_NAME: &str = "crate-type";

pub fn cli() -> App {
subcommand("rustc")
.setting(AppSettings::TrailingVarArg)
.trailing_var_arg(true)
.about("Compile a package, and pass extra options to the compiler")
.arg_quiet()
.arg(Arg::new("args").multiple_values(true).help("Rustc flags"))
Expand Down
2 changes: 1 addition & 1 deletion src/bin/cargo/commands/rustdoc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::command_prelude::*;

pub fn cli() -> App {
subcommand("rustdoc")
.setting(AppSettings::TrailingVarArg)
.trailing_var_arg(true)
.about("Build a package's documentation, using specified custom flags.")
.arg_quiet()
.arg(Arg::new("args").multiple_values(true))
Expand Down
2 changes: 1 addition & 1 deletion src/bin/cargo/commands/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ pub fn cli() -> App {
subcommand("test")
// Subcommand aliases are handled in `aliased_command()`.
// .alias("t")
.setting(AppSettings::TrailingVarArg)
.trailing_var_arg(true)
.about("Execute all unit and integration tests and build examples of a local package")
.arg(
Arg::new("TESTNAME")
Expand Down
6 changes: 4 additions & 2 deletions src/cargo/util/command_prelude.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ pub use crate::core::compiler::CompileMode;
pub use crate::{CliError, CliResult, Config};
pub use clap::{AppSettings, Arg, ArgMatches};

pub type App = clap::App<'static>;
pub type App = clap::Command<'static>;

pub trait AppExt: Sized {
fn _arg(self, arg: Arg<'static>) -> Self;
Expand Down Expand Up @@ -281,7 +281,9 @@ pub fn multi_opt(name: &'static str, value_name: &'static str, help: &'static st
}

pub fn subcommand(name: &'static str) -> App {
App::new(name).setting(AppSettings::DeriveDisplayOrder | AppSettings::DontCollapseArgsInUsage)
App::new(name)
.dont_collapse_args_in_usage(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI this became a global_setting.

If that is a concern, you can still use the AppSetting until clap v4. In clap v4, my expectation is we'll support subcommands overriding global settings (I think that doesn't work today)

When we made this global, it was under the assumption people would want a uniform look. If this assumption is incorrect, we can open an issue and re-visit this in clap v4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, interesting. I honestly couldn't tell you whether that change is okay or not. I feel like it probably is since we're seemingly applying it to all subcommands, but others who know the CLI interface better may need to weigh in here.

.setting(AppSettings::DeriveDisplayOrder)
}

/// Determines whether or not to gate `--profile` as unstable when resolving it.
Expand Down