Skip to content

Commit

Permalink
Merge pull request #5298 from epage/conflict
Browse files Browse the repository at this point in the history 
fix(parser): Improve `args_conflicts_with_subcommands` experience
  • Loading branch information
@epage
epage authored Jan 11, 2024
2 parents 514f28b + f529ec3 commit a5d4641
Show file tree
Hide file tree
Showing 4 changed files with 235 additions and 70 deletions.
62 changes: 36 additions & 26 deletions clap_builder/src/error/format.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,12 +146,10 @@ fn write_dynamic_context(

match error.kind() {
ErrorKind::ArgumentConflict => {
let invalid_arg = error.get(ContextKind::InvalidArg);
let prior_arg = error.get(ContextKind::PriorArg);
if let (Some(ContextValue::String(invalid_arg)), Some(prior_arg)) =
(invalid_arg, prior_arg)
{
if ContextValue::String(invalid_arg.clone()) == *prior_arg {
let mut prior_arg = error.get(ContextKind::PriorArg);
if let Some(ContextValue::String(invalid_arg)) = error.get(ContextKind::InvalidArg) {
if Some(&ContextValue::String(invalid_arg.clone())) == prior_arg {
prior_arg = None;
let _ = write!(
styled,
"the argument '{}{invalid_arg}{}' cannot be used multiple times",
Expand All @@ -165,36 +163,48 @@ fn write_dynamic_context(
invalid.render(),
invalid.render_reset()
);
}
} else if let Some(ContextValue::String(invalid_arg)) =
error.get(ContextKind::InvalidSubcommand)
{
let _ = write!(
styled,
"the subcommand '{}{invalid_arg}{}' cannot be used with",
invalid.render(),
invalid.render_reset()
);
} else {
styled.push_str(error.kind().as_str().unwrap());
}

match prior_arg {
ContextValue::Strings(values) => {
styled.push_str(":");
for v in values {
let _ = write!(
styled,
"\n{TAB}{}{v}{}",
invalid.render(),
invalid.render_reset()
);
}
}
ContextValue::String(value) => {
if let Some(prior_arg) = prior_arg {
match prior_arg {
ContextValue::Strings(values) => {
styled.push_str(":");
for v in values {
let _ = write!(
styled,
" '{}{value}{}'",
"\n{TAB}{}{v}{}",
invalid.render(),
invalid.render_reset()
);
}
_ => {
styled.push_str(" one or more of the other specified arguments");
}
}
ContextValue::String(value) => {
let _ = write!(
styled,
" '{}{value}{}'",
invalid.render(),
invalid.render_reset()
);
}
_ => {
styled.push_str(" one or more of the other specified arguments");
}
}
true
} else {
false
}

true
}
ErrorKind::NoEquals => {
let invalid_arg = error.get(ContextKind::InvalidArg);
Expand Down
28 changes: 28 additions & 0 deletions clap_builder/src/error/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,34 @@ impl<F: ErrorFormatter> Error<F> {
err
}

pub(crate) fn subcommand_conflict(
cmd: &Command,
sub: String,
mut others: Vec<String>,
usage: Option<StyledStr>,
) -> Self {
let mut err = Self::new(ErrorKind::ArgumentConflict).with_cmd(cmd);

#[cfg(feature = "error-context")]
{
let others = match others.len() {
0 => ContextValue::None,
1 => ContextValue::String(others.pop().unwrap()),
_ => ContextValue::Strings(others),
};
err = err.extend_context_unchecked([
(ContextKind::InvalidSubcommand, ContextValue::String(sub)),
(ContextKind::PriorArg, others),
]);
if let Some(usage) = usage {
err = err
.insert_context_unchecked(ContextKind::Usage, ContextValue::StyledStr(usage));
}
}

err
}

pub(crate) fn empty_value(cmd: &Command, good_vals: &[String], arg: String) -> Self {
Self::invalid_value(cmd, "".to_owned(), good_vals, arg)
}
Expand Down
37 changes: 32 additions & 5 deletions clap_builder/src/parser/parser.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -444,11 +444,27 @@ impl<'cmd> Parser<'cmd> {
} else {
// Start error processing
let _ = self.resolve_pending(matcher);
return Err(self.match_arg_error(&arg_os, valid_arg_found, trailing_values));
return Err(self.match_arg_error(
&arg_os,
valid_arg_found,
trailing_values,
matcher,
));
}
}

if let Some(ref pos_sc_name) = subcmd_name {
if self.cmd.is_args_conflicts_with_subcommands_set() && valid_arg_found {
return Err(ClapError::subcommand_conflict(
self.cmd,
pos_sc_name.clone(),
matcher
.arg_ids()
.map(|id| self.cmd.find(id).unwrap().to_string())
.collect(),
Usage::new(self.cmd).create_usage_with_title(&[]),
));
}
let sc_name = self
.cmd
.find_subcommand(pos_sc_name)
Expand All @@ -470,6 +486,7 @@ impl<'cmd> Parser<'cmd> {
arg_os: &clap_lex::ParsedArg<'_>,
valid_arg_found: bool,
trailing_values: bool,
matcher: &ArgMatcher,
) -> ClapError {
// If argument follows a `--`
if trailing_values {
Expand All @@ -490,7 +507,19 @@ impl<'cmd> Parser<'cmd> {
&& self.cmd.has_positionals()
&& (arg_os.is_long() || arg_os.is_short());

if !(self.cmd.is_args_conflicts_with_subcommands_set() && valid_arg_found) {
if self.cmd.has_subcommands() {
if self.cmd.is_args_conflicts_with_subcommands_set() && valid_arg_found {
return ClapError::subcommand_conflict(
self.cmd,
arg_os.display().to_string(),
matcher
.arg_ids()
.map(|id| self.cmd.find(id).unwrap().to_string())
.collect(),
Usage::new(self.cmd).create_usage_with_title(&[]),
);
}

let candidates = suggestions::did_you_mean(
&arg_os.display().to_string(),
self.cmd.all_subcommand_names(),
Expand All @@ -508,9 +537,7 @@ impl<'cmd> Parser<'cmd> {
}

// If the argument must be a subcommand.
if self.cmd.has_subcommands()
&& (!self.cmd.has_positionals() || self.cmd.is_infer_subcommands_set())
{
if !self.cmd.has_positionals() || self.cmd.is_infer_subcommands_set() {
return ClapError::unrecognized_subcommand(
self.cmd,
arg_os.display().to_string(),
Expand Down
178 changes: 139 additions & 39 deletions tests/builder/conflicts.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -684,6 +684,145 @@ fn exclusive_with_required() {
cmd.clone().try_get_matches_from(["bug", "--test"]).unwrap();
}

#[test]
#[cfg(feature = "error-context")]
fn option_conflicts_with_subcommand() {
static CONFLICT_ERR: &str = "\
error: the subcommand 'sub1' cannot be used with '--place <place id>'
Usage: test [OPTIONS]
test <COMMAND>
For more information, try '--help'.
";

let cmd = Command::new("test")
.args_conflicts_with_subcommands(true)
.arg(arg!(-p --place <"place id"> "Place ID to open"))
.subcommand(Command::new("sub1"));

utils::assert_output(cmd, "test --place id sub1", CONFLICT_ERR, true);
}

#[test]
#[cfg(feature = "error-context")]
fn positional_conflicts_with_subcommand() {
static CONFLICT_ERR: &str = "\
error: the subcommand 'sub1' cannot be used with '<arg1>'
Usage: test <arg1>
test <COMMAND>
For more information, try '--help'.
";

let cmd = Command::new("test")
.args_conflicts_with_subcommands(true)
.arg(arg!(<arg1> "some arg"))
.subcommand(Command::new("sub1"));

utils::assert_output(cmd, "test value1 sub1", CONFLICT_ERR, true);
}

#[test]
#[cfg(feature = "error-context")]
fn flag_conflicts_with_subcommand_long_flag() {
static CONFLICT_ERR: &str = "\
error: the subcommand 'sub' cannot be used with '--hello'
Usage: test [OPTIONS]
test <COMMAND>
For more information, try '--help'.
";

let cmd = Command::new("test")
.args_conflicts_with_subcommands(true)
.arg(arg!(--hello))
.subcommand(Command::new("sub").long_flag("sub"));

utils::assert_output(cmd, "test --hello --sub", CONFLICT_ERR, true);
}

#[test]
#[cfg(feature = "error-context")]
fn flag_conflicts_with_subcommand_short_flag() {
static CONFLICT_ERR: &str = "\
error: the subcommand 'sub' cannot be used with '--hello'
Usage: test [OPTIONS]
test <COMMAND>
For more information, try '--help'.
";

let cmd = Command::new("test")
.args_conflicts_with_subcommands(true)
.arg(arg!(--hello))
.subcommand(Command::new("sub").short_flag('s'));

utils::assert_output(cmd, "test --hello -s", CONFLICT_ERR, true);
}

#[test]
#[cfg(feature = "error-context")]
fn positional_conflicts_with_subcommand_precedent() {
static CONFLICT_ERR: &str = "\
error: the subcommand 'sub' cannot be used with '<arg1>'
Usage: test <arg1>
test <COMMAND>
For more information, try '--help'.
";

let cmd = Command::new("test")
.args_conflicts_with_subcommands(true)
.subcommand_precedence_over_arg(true)
.arg(arg!(<arg1> "some arg"))
.subcommand(Command::new("sub"));

utils::assert_output(cmd, "test hello sub", CONFLICT_ERR, true);
}

#[test]
#[cfg(feature = "error-context")]
fn flag_conflicts_with_subcommand_precedent() {
static CONFLICT_ERR: &str = "\
error: the subcommand 'sub' cannot be used with '--hello'
Usage: test [OPTIONS]
test <COMMAND>
For more information, try '--help'.
";

let cmd = Command::new("test")
.args_conflicts_with_subcommands(true)
.subcommand_precedence_over_arg(true)
.arg(arg!(--hello))
.subcommand(Command::new("sub"));

utils::assert_output(cmd, "test --hello sub", CONFLICT_ERR, true);
}

#[test]
fn subcommand_conflict_negates_required() {
let cmd = Command::new("test")
.args_conflicts_with_subcommands(true)
.subcommand(Command::new("config"))
.arg(arg!(-p --place <"place id"> "Place ID to open").required(true));

let result = cmd.try_get_matches_from(["test", "config"]);
assert!(
result.is_ok(),
"args_conflicts_with_subcommands should ignore required: {}",
result.unwrap_err()
);
let m = result.unwrap();
assert_eq!(m.subcommand_name().unwrap(), "config");
}

#[test]
fn args_negate_subcommands_one_level() {
let res = Command::new("disablehelp")
Expand Down Expand Up @@ -729,42 +868,3 @@ fn args_negate_subcommands_two_levels() {
Some("sub2")
);
}

#[test]
#[cfg(feature = "error-context")]
fn subcommand_conflict_error_message() {
static CONFLICT_ERR: &str = "\
error: unexpected argument 'sub1' found
Usage: test [OPTIONS]
test <COMMAND>
For more information, try '--help'.
";

let cmd = Command::new("test")
.args_conflicts_with_subcommands(true)
.arg(arg!(-p --place <"place id"> "Place ID to open"))
.subcommand(
Command::new("sub1").subcommand(Command::new("sub2").subcommand(Command::new("sub3"))),
);

utils::assert_output(cmd, "test --place id sub1", CONFLICT_ERR, true);
}

#[test]
fn subcommand_conflict_negates_required() {
let cmd = Command::new("test")
.args_conflicts_with_subcommands(true)
.subcommand(Command::new("config"))
.arg(arg!(-p --place <"place id"> "Place ID to open").required(true));

let result = cmd.try_get_matches_from(["test", "config"]);
assert!(
result.is_ok(),
"args_conflicts_with_subcommands should ignore required: {}",
result.unwrap_err()
);
let m = result.unwrap();
assert_eq!(m.subcommand_name().unwrap(), "config");
}

0 comments on commit a5d4641

Please sign in to comment.