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

Fix a regression with parsing multivalue options #5196

Merged
merged 1 commit into from
Mar 16, 2018
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
1 change: 1 addition & 0 deletions src/bin/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ See 'cargo help <command>' for more information on a specific command.",
.short("Z")
.value_name("FLAG")
.multiple(true)
.number_of_values(1)
.global(true),
)
.subcommands(commands::builtin());
Expand Down
54 changes: 37 additions & 17 deletions src/bin/command_prelude.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,22 @@ pub type App = clap::App<'static, 'static>;
pub trait AppExt: Sized {
fn _arg(self, arg: Arg<'static, 'static>) -> Self;

fn arg_package(self, package: &'static str, all: &'static str, exclude: &'static str) -> Self {
self._arg(
opt("package", package)
.short("p")
.value_name("SPEC")
.multiple(true),
)._arg(opt("all", all))
._arg(opt("exclude", exclude).value_name("SPEC").multiple(true))
fn arg_package_spec(
self,
package: &'static str,
all: &'static str,
exclude: &'static str,
) -> Self {
self.arg_package_spec_simple(package)
._arg(opt("all", all))
._arg(multi_opt("exclude", "SPEC", exclude))
}

fn arg_single_package(self, package: &'static str) -> Self {
fn arg_package_spec_simple(self, package: &'static str) -> Self {
self._arg(multi_opt("package", "SPEC", package).short("p"))
}

fn arg_package(self, package: &'static str) -> Self {
self._arg(opt("package", package).short("p").value_name("SPEC"))
}

Expand All @@ -51,18 +56,18 @@ pub trait AppExt: Sized {
all: &'static str,
) -> Self {
self.arg_targets_lib_bin(lib, bin, bins)
._arg(opt("example", examle).value_name("NAME").multiple(true))
._arg(multi_opt("example", "NAME", examle))
._arg(opt("examples", examles))
._arg(opt("test", test).value_name("NAME").multiple(true))
._arg(multi_opt("test", "NAME", test))
._arg(opt("tests", tests))
._arg(opt("bench", bench).value_name("NAME").multiple(true))
._arg(multi_opt("bench", "NAME", bench))
._arg(opt("benches", benchs))
._arg(opt("all-targets", all))
}

fn arg_targets_lib_bin(self, lib: &'static str, bin: &'static str, bins: &'static str) -> Self {
self._arg(opt("lib", lib))
._arg(opt("bin", bin).value_name("NAME").multiple(true))
._arg(multi_opt("bin", "NAME", bin))
._arg(opt("bins", bins))
}

Expand All @@ -73,15 +78,15 @@ pub trait AppExt: Sized {
example: &'static str,
examples: &'static str,
) -> Self {
self._arg(opt("bin", bin).value_name("NAME").multiple(true))
self._arg(multi_opt("bin", "NAME", bin))
._arg(opt("bins", bins))
._arg(opt("example", example).value_name("NAME").multiple(true))
._arg(multi_opt("example", "NAME", example))
._arg(opt("examples", examples))
}

fn arg_targets_bin_example(self, bin: &'static str, example: &'static str) -> Self {
self._arg(opt("bin", bin).value_name("NAME").multiple(true))
._arg(opt("example", example).value_name("NAME").multiple(true))
self._arg(multi_opt("bin", "NAME", bin))
._arg(multi_opt("example", "NAME", example))
}

fn arg_features(self) -> Self {
Expand Down Expand Up @@ -157,6 +162,21 @@ pub fn opt(name: &'static str, help: &'static str) -> Arg<'static, 'static> {
Arg::with_name(name).long(name).help(help)
}

pub fn multi_opt(
name: &'static str,
value_name: &'static str,
help: &'static str,
) -> Arg<'static, 'static> {
// Note that all `.multiple(true)` arguments in Cargo should specify
// `.number_of_values(1)` as well, so that `--foo val1 val2` is
// **not** parsed as `foo` with values ["val1", "val2"].
// `number_of_values` should become the default in clap 3.
opt(name, help)
.value_name(value_name)
.multiple(true)
.number_of_values(1)
}

pub fn subcommand(name: &'static str) -> App {
SubCommand::with_name(name).settings(&[
AppSettings::UnifiedHelpMessage,
Expand Down
2 changes: 1 addition & 1 deletion src/bin/commands/bench.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ pub fn cli() -> App {
"Benchmark all targets (default)",
)
.arg(opt("no-run", "Compile, but don't run benchmarks"))
.arg_package(
.arg_package_spec(
"Package to run benchmarks for",
"Benchmark all packages in the workspace",
"Exclude packages from the benchmark",
Expand Down
2 changes: 1 addition & 1 deletion src/bin/commands/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ pub fn cli() -> App {
subcommand("build")
.alias("b")
.about("Compile a local package and all of its dependencies")
.arg_package(
.arg_package_spec(
"Package to build",
"Build all packages in the workspace",
"Exclude packages from the build",
Expand Down
2 changes: 1 addition & 1 deletion src/bin/commands/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use cargo::ops::{self, CompileMode};
pub fn cli() -> App {
subcommand("check")
.about("Check a local package and all of its dependencies for errors")
.arg_package(
.arg_package_spec(
"Package(s) to check",
"Check all packages in the workspace",
"Exclude packages from the check",
Expand Down
7 changes: 1 addition & 6 deletions src/bin/commands/clean.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,7 @@ use cargo::ops::{self, CleanOptions};
pub fn cli() -> App {
subcommand("clean")
.about("Remove artifacts that cargo has generated in the past")
.arg(
opt("package", "Package to clean artifacts for")
.short("p")
.value_name("SPEC")
.multiple(true),
)
.arg_package_spec_simple("Package to clean artifacts for")
.arg_manifest_path()
.arg_target_triple("Target triple to clean output for (default all)")
.arg_release("Whether or not to clean release artifacts")
Expand Down
2 changes: 1 addition & 1 deletion src/bin/commands/doc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ pub fn cli() -> App {
"open",
"Opens the docs in a browser after the operation",
))
.arg_package(
.arg_package_spec(
"Package to document",
"Document all packages in the workspace",
"Exclude packages from the build",
Expand Down
16 changes: 6 additions & 10 deletions src/bin/commands/owner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,13 @@ pub fn cli() -> App {
subcommand("owner")
.about("Manage the owners of a crate on the registry")
.arg(Arg::with_name("crate"))
.arg(multi_opt("add", "LOGIN", "Name of a user or team to add as an owner").short("a"))
.arg(
opt("add", "Name of a user or team to add as an owner")
.short("a")
.value_name("LOGIN")
.multiple(true),
)
.arg(
opt("remove", "Name of a user or team to remove as an owner")
.short("r")
.value_name("LOGIN")
.multiple(true),
multi_opt(
"remove",
"LOGIN",
"Name of a user or team to remove as an owner",
).short("r"),
)
.arg(opt("list", "List owners of a crate").short("l"))
.arg(opt("index", "Registry index to modify owners for").value_name("INDEX"))
Expand Down
2 changes: 1 addition & 1 deletion src/bin/commands/pkgid.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ pub fn cli() -> App {
subcommand("pkgid")
.about("Print a fully qualified package specification")
.arg(Arg::with_name("spec"))
.arg_single_package("Argument to get the package id specifier for")
.arg_package("Argument to get the package id specifier for")
.arg_manifest_path()
.after_help(
"\
Expand Down
2 changes: 1 addition & 1 deletion src/bin/commands/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ pub fn cli() -> App {
"Name of the bin target to run",
"Name of the example target to run",
)
.arg_single_package("Package with the target to run")
.arg_package("Package with the target to run")
.arg_jobs()
.arg_release("Build artifacts in release mode, with optimizations")
.arg_features()
Expand Down
2 changes: 1 addition & 1 deletion src/bin/commands/rustc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ pub fn cli() -> App {
.setting(AppSettings::TrailingVarArg)
.about("Compile a package and all of its dependencies")
.arg(Arg::with_name("args").multiple(true))
.arg_single_package("Package to build")
.arg_package("Package to build")
.arg_jobs()
.arg_targets_all(
"Build only this package's library",
Expand Down
2 changes: 1 addition & 1 deletion src/bin/commands/rustdoc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ pub fn cli() -> App {
"open",
"Opens the docs in a browser after the operation",
))
.arg_single_package("Package to document")
.arg_package("Package to document")
.arg_jobs()
.arg_targets_all(
"Build only this package's library",
Expand Down
2 changes: 1 addition & 1 deletion src/bin/commands/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ pub fn cli() -> App {
.arg(opt("doc", "Test only this library's documentation"))
.arg(opt("no-run", "Compile, but don't run tests"))
.arg(opt("no-fail-fast", "Run all tests regardless of failure"))
.arg_package(
.arg_package_spec(
"Package to run tests for",
"Test all packages in the workspace",
"Exclude packages from the test",
Expand Down
6 changes: 1 addition & 5 deletions src/bin/commands/uninstall.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,7 @@ pub fn cli() -> App {
subcommand("uninstall")
.about("Remove a Rust binary")
.arg(Arg::with_name("spec").multiple(true))
.arg(
opt("bin", "Only uninstall the binary NAME")
.value_name("NAME")
.multiple(true),
)
.arg(multi_opt("bin", "NAME", "Only uninstall the binary NAME"))
.arg(opt("root", "Directory to uninstall packages from").value_name("DIR"))
.after_help(
"\
Expand Down
7 changes: 1 addition & 6 deletions src/bin/commands/update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,7 @@ use cargo::ops::{self, UpdateOptions};
pub fn cli() -> App {
subcommand("update")
.about("Update dependencies as recorded in the local lock file")
.arg(
opt("package", "Package to clean artifacts for")
.short("p")
.value_name("SPEC")
.multiple(true),
)
.arg_package_spec_simple("Package to update")
.arg(opt(
"aggressive",
"Force updating all dependencies of <name> as well",
Expand Down
33 changes: 33 additions & 0 deletions tests/testsuite/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1109,3 +1109,36 @@ fn run_multiple_packages() {
.with_stderr_contains("[ERROR] package `d3` is not a member of the workspace"),
);
}

#[test]
fn explicit_bin_with_args() {
let p = project("foo")
.file(
"Cargo.toml",
r#"
[project]
name = "foo"
version = "0.0.1"
authors = []
"#,
)
.file(
"src/main.rs",
r#"
fn main() {
assert_eq!(std::env::args().nth(1).unwrap(), "hello");
assert_eq!(std::env::args().nth(2).unwrap(), "world");
}
"#,
)
.build();

assert_that(
p.cargo("run")
.arg("--bin")
.arg("foo")
.arg("hello")
.arg("world"),
execs().with_status(0),
);
Copy link
Member Author

Choose a reason for hiding this comment

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

Not related to the PR, but do you feel like making p.cargo a little bit more magical, so that

    assert_that(
        p.cargo("run --bin foo hello world"),
        execs().with_status(0),
    );

works?

Copy link
Member

Choose a reason for hiding this comment

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

I'd be totally down for that!

}