Mirrors/clap
2
0
Fork 0
mirror of https://github.com/clap-rs/clap synced 2024-12-14 06:42:33 +00:00
Code Issues Projects Releases Packages Wiki Activity

Merge #1856

Browse source 
1856: Fix positional args in groups (#1794) r=pksunkara a=CreepySkeleton



Co-authored-by: CreepySkeleton <creepy-skeleton@yandex.ru>
This commit is contained in:
bors[bot] 2020-04-24 10:59:11 +00:00 committed by GitHub
commit 7b5e2cf897
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 161 additions and 26 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
src/build/arg/mod.rs
View file

@ -4143,6 +4143,10 @@ impl<'help> Arg<'help> {
self.is_set(ArgSettings::TakesValue) || self.long.is_some() || self.short.is_none()
}
pub(crate) fn is_positional(&self) -> bool {
self.long.is_none() && self.short.is_none()
}
// Used for positionals when printing
pub(crate) fn multiple_str(&self) -> &str {
let mult_vals = self

101
src/parse/parser.rs
View file

@ -22,13 +22,13 @@ use crate::INVALID_UTF8;
#[derive(Debug, PartialEq, Clone)]
pub(crate) enum ParseResult {
Flag,
Flag(Id),
Opt(Id),
Pos(Id),
MaybeHyphenValue,
MaybeNegNum,
NotFound,
ValuesDone,
ValuesDone(Id),
}
#[derive(Debug)]
@ -494,9 +494,13 @@ where
needs_val_of
);
match needs_val_of {
ParseResult::Flag | ParseResult::Opt(..) | ParseResult::ValuesDone => {
continue
ParseResult::Flag(ref id)
| ParseResult::Opt(ref id)
| ParseResult::ValuesDone(ref id) => {
self.maybe_inc_pos_counter(&mut pos_counter, id);
continue;
}
_ => (),
}
} else if arg_os.starts_with("-") && arg_os.len() != 1 {
@ -523,8 +527,11 @@ where
)?);
}
}
ParseResult::Opt(..) | ParseResult::Flag | ParseResult::ValuesDone => {
continue
ParseResult::Opt(ref id)
| ParseResult::Flag(ref id)
| ParseResult::ValuesDone(ref id) => {
self.maybe_inc_pos_counter(&mut pos_counter, id);
continue;
}
_ => (),
}
@ -590,16 +597,18 @@ where
if low_index_mults || missing_pos {
if let Some(n) = next_arg {
needs_val_of = if needs_val_of != ParseResult::ValuesDone {
needs_val_of = match needs_val_of {
ParseResult::ValuesDone(id) => ParseResult::ValuesDone(id),
_ => {
if let Some(p) =
positionals!(self.app).find(|p| p.index == Some(pos_counter as u64))
{
ParseResult::Pos(p.id.clone())
} else {
ParseResult::ValuesDone
ParseResult::ValuesDone(Id::empty_hash())
}
}
} else {
ParseResult::ValuesDone
};
let n = ArgStr::new(n);
@ -620,7 +629,7 @@ where
} else if (self.is_set(AS::AllowMissingPositional) && self.is_set(AS::TrailingValues))
|| (self.is_set(AS::ContainsLast) && self.is_set(AS::TrailingValues))
{
// Came to -- and one postional has .last(true) set, so we go immediately
// Came to -- and one positional has .last(true) set, so we go immediately
// to the last (highest index) positional
debug!("Parser::get_matches_with: .last(true) and --, setting last pos");
pos_counter = self
@ -802,6 +811,58 @@ where
}
}
// HACK:
// When we have a group that is NOT multiple AND has both
// an option and a positional arg, only one of them must be passed.
//
// The problem here is that clap decides which positional arg it's looking at
// (the one from group or the following one) based on the counter that is incremented
// every time clap stores a positional arg.
//
// When a non positional option is passed, this counter is not incremented.
// In other words, the group has already been "occupied" by the option, but the
// counter still points to the positional argument that belongs to this group.
// If the option is followed by yet another positional arg, it will land in
// the group, erroneously.
//
// This is a pretty much fundamental problem with the current parsing algorithm
// and the function below is simply a hack that solves the problem at hand,
// but it does so at cost of minor regression in error messages (see tests/groups.rs).
//
// In order to resolve it properly, we need to rewrite the parser entirely.
// I don't really feel like it right now, sorry.
// The parser is big and scary and full of legacy.
fn maybe_inc_pos_counter(&self, pos_counter: &mut usize, id: &Id) {
debug!("Parser::maybe_inc_pos_counter: arg = {:?}", id);
let arg = self.app.find(id).expect(INTERNAL_ERROR_MSG);
debug!("Parser::maybe_inc_pos_counter: is it positional?");
// will be incremented by other means.
if arg.is_positional() {
debug!("Yes");
return;
}
debug!("No");
for group in groups_for_arg!(self.app, arg.id.clone()) {
debug!("Parser::maybe_inc_pos_counter: group={:?}", group);
let group = self.app.groups.iter().find(|g| g.id == group);
debug!("Parser::maybe_inc_pos_counter: Checking args in the group...");
if let Some(group) = group {
for arg in group.args.iter() {
debug!("Parser::maybe_inc_pos_counter: arg={:?}", arg);
let arg = self.app.find(arg).expect(INTERNAL_ERROR_MSG);
if arg.is_positional() {
debug!("Parser::maybe_inc_pos_counter: Incrementing counter!");
*pos_counter += 1;
}
}
}
}
}
// Checks if the arg matches a subcommand name, or any of it's aliases (if defined)
fn possible_subcommand(&self, arg_os: &ArgStr<'_>) -> Option<&str> {
debug!("Parser::possible_subcommand: arg={:?}", arg_os);
@ -933,7 +994,7 @@ where
let p = self.app.find(&name).expect(INTERNAL_ERROR_MSG);
p.is_set(ArgSettings::AllowHyphenValues) || app_wide_settings
}
ParseResult::ValuesDone => return true,
ParseResult::ValuesDone(..) => return true,
_ => false,
};
@ -1139,7 +1200,7 @@ where
self.check_for_help_and_version_str(&arg)?;
self.parse_flag(opt, matcher)?;
return Ok(ParseResult::Flag);
return Ok(ParseResult::Flag(opt.id.clone()));
} else if self.is_set(AS::AllowLeadingHyphen) {
return Ok(ParseResult::MaybeHyphenValue);
} else if self.is_set(AS::ValidNegNumFound) {
@ -1293,13 +1354,13 @@ where
// @TODO @soundness: if doesn't have an equal, but requires equal is ValuesDone?!
if no_val && min_vals_zero && !has_eq && needs_eq {
debug!("Parser::parse_opt: More arg vals not required...");
return Ok(ParseResult::ValuesDone);
return Ok(ParseResult::ValuesDone(opt.id.clone()));
} else if no_val || (mult && !needs_delim) && !has_eq && matcher.needs_more_vals(opt) {
debug!("Parser::parse_opt: More arg vals required...");
return Ok(ParseResult::Opt(opt.id.clone()));
}
debug!("Parser::parse_opt: More arg vals not required...");
Ok(ParseResult::ValuesDone)
Ok(ParseResult::ValuesDone(opt.id.clone()))
}
fn add_val_to_arg(
@ -1319,13 +1380,13 @@ where
if val.is_empty() {
Ok(self.add_single_val_to_arg(arg, val, matcher)?)
} else {
let mut iret = ParseResult::ValuesDone;
let mut iret = ParseResult::ValuesDone(arg.id.clone());
for v in val.split(delim) {
iret = self.add_single_val_to_arg(arg, &v, matcher)?;
}
// If there was a delimiter used, we're not looking for more values
if val.contains_char(delim) || arg.is_set(ArgSettings::RequireDelimiter) {
iret = ParseResult::ValuesDone;
iret = ParseResult::ValuesDone(arg.id.clone());
}
Ok(iret)
}
@ -1351,7 +1412,7 @@ where
// @TODO @docs @p4 docs should probably note that terminator doesn't get an index
if let Some(t) = arg.terminator {
if t == v {
return Ok(ParseResult::ValuesDone);
return Ok(ParseResult::ValuesDone(arg.id.clone()));
}
}
@ -1366,7 +1427,7 @@ where
if matcher.needs_more_vals(arg) {
return Ok(ParseResult::Opt(arg.id.clone()));
}
Ok(ParseResult::ValuesDone)
Ok(ParseResult::ValuesDone(arg.id.clone()))
}
fn parse_flag(&self, flag: &Arg<'b>, matcher: &mut ArgMatcher) -> ClapResult<ParseResult> {
@ -1379,7 +1440,7 @@ where
matcher.inc_occurrence_of(&grp);
}
Ok(ParseResult::Flag)
Ok(ParseResult::Flag(flag.id.clone()))
}
fn remove_overrides(&mut self, matcher: &mut ArgMatcher) {

72
tests/groups.rs
View file

@ -18,6 +18,7 @@ USAGE:
For more information try --help";
#[allow(unused)]
static REQ_GROUP_CONFLICT_REV: &str = "error: The argument '--delete' cannot be used with '<base>'
USAGE:
@ -25,6 +26,28 @@ USAGE:
For more information try --help";
static REQ_GROUP_CONFLICT_ONLY_OPTIONS: &str =
"error: Found argument '--all' which wasn't expected, or isn't valid in this context
If you tried to supply `--all` as a PATTERN use `-- --all`
USAGE:
clap-test <-a|--delete>
For more information try --help";
// FIXME: This message has regressed after https://github.com/clap-rs/clap/pull/1856
// Need to roll back somehow.
static REQ_GROUP_CONFLICT_REV_DEGRADED: &str =
"error: Found argument 'base' which wasn't expected, or isn't valid in this context
If you tried to supply `base` as a PATTERN use `-- base`
USAGE:
clap-test <base|--delete>
For more information try --help";
#[test]
fn required_group_missing_arg() {
let result = App::new("group")
@ -208,7 +231,28 @@ fn req_group_with_conflict_usage_string() {
assert!(utils::compare_output2(
app,
"clap-test --delete base",
REQ_GROUP_CONFLICT_REV,
REQ_GROUP_CONFLICT_REV_DEGRADED,
REQ_GROUP_CONFLICT_USAGE,
true
));
}
#[test]
fn req_group_with_conflict_usage_string_only_options() {
let app = App::new("req_group")
.arg(Arg::from("<all> -a, -all 'All'").conflicts_with("delete"))
.arg(Arg::from(
"<delete> -d, --delete 'Remove the base commit information'",
))
.group(
ArgGroup::with_name("all_or_delete")
.args(&["all", "delete"])
.required(true),
);
assert!(utils::compare_output2(
app,
"clap-test --delete --all",
REQ_GROUP_CONFLICT_ONLY_OPTIONS,
REQ_GROUP_CONFLICT_USAGE,
true
));
@ -252,3 +296,29 @@ fn group_acts_like_arg() {
.get_matches_from(vec!["prog", "--debug"]);
assert!(m.is_present("mode"));
}
#[test]
fn issue_1794() {
let app = clap::App::new("hello")
.bin_name("deno")
.arg(Arg::with_name("option1").long("option1").takes_value(false))
.arg(Arg::with_name("pos1").takes_value(true))
.arg(Arg::with_name("pos2").takes_value(true))
.group(
ArgGroup::with_name("arg1")
.args(&["pos1", "option1"])
.required(true),
);
let m = app.clone().get_matches_from(&["app", "pos1", "pos2"]);
assert_eq!(m.value_of("pos1"), Some("pos1"));
assert_eq!(m.value_of("pos2"), Some("pos2"));
assert!(!m.is_present("option1"));
let m = app
.clone()
.get_matches_from(&["app", "--option1", "positional"]);
assert_eq!(m.value_of("pos1"), None);
assert_eq!(m.value_of("pos2"), Some("positional"));
assert!(m.is_present("option1"));
}