Skip to content

Commit

Permalink
Merge pull request #302 from pacak/scan-pos
Browse files Browse the repository at this point in the history
Scan for positional items instead of trying to take the next available
  • Loading branch information
pacak authored Sep 30, 2023
2 parents 748138f + 54805b2 commit c919d73
Show file tree
Hide file tree
Showing 9 changed files with 76 additions and 53 deletions.
1 change: 1 addition & 0 deletions Changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
## bpaf [0.9.6], bpaf_derive [0.5.6] - Unreleased
- make sure env-only arguments and flags are working
- support raw identifiers in derive macro (#282)
- better error messages for unexpected values that prevent positional parses

## bpaf [0.9.5], bpaf_derive [0.5.5] - 2023-08-24
- fancier squashing: parse `-abfoo` as `-a -b=foo` if b is a short argument
Expand Down
37 changes: 17 additions & 20 deletions src/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -676,33 +676,30 @@ impl State {
pub(crate) fn take_positional_word(
&mut self,
metavar: Metavar,
) -> Result<Option<(usize, bool, OsString)>, Error> {
#[allow(clippy::range_plus_one)] // gives wrong type otherwise
match self.items_iter().next() {
Some((ix, Arg::PosWord(w))) => {
) -> Result<(usize, bool, OsString), Error> {
match self.items_iter().find_map(|(ix, arg)| match arg {
Arg::Word(w) => Some((ix, false, w)),
Arg::PosWord(w) => Some((ix, true, w)),
_ => None,
}) {
Some((ix, strict, w)) => {
let w = w.clone();
self.current = Some(ix);
self.remove(ix);
Ok(Some((ix, true, w)))
Ok((ix, strict, w))
}
Some((ix, Arg::Word(w))) => {
let w = w.clone();
self.current = Some(ix);
self.remove(ix);
Ok(Some((ix, false, w)))
}
Some((position, _arg)) => {
None => {
let scope = self.scope();
let missing = MissingItem {
item: Item::Positional {
help: None,
metavar,
},
position,
scope: position..position + 1,
position: scope.start,
scope,
};
Err(Error(Message::Missing(vec![missing])))
}
None => Ok(None),
}
}

Expand Down Expand Up @@ -760,7 +757,7 @@ mod tests {
let flag = a.take_flag(&long("speed"));
assert!(flag);
assert!(!a.is_empty());
let s = a.take_positional_word(M).unwrap().unwrap();
let s = a.take_positional_word(M).unwrap();
assert_eq!(s.2, "12");
assert!(a.is_empty());
}
Expand Down Expand Up @@ -856,7 +853,7 @@ mod tests {
fn command_and_positional() {
let mut a = State::from(&["cmd", "pos"]);
assert!(a.take_cmd("cmd"));
let w = a.take_positional_word(M).unwrap().unwrap();
let w = a.take_positional_word(M).unwrap();
assert_eq!(w.2, "pos");
assert!(a.is_empty());
}
Expand All @@ -865,7 +862,7 @@ mod tests {
fn positionals_after_double_dash1() {
let mut a = State::from(&["-v", "--", "-x"]);
assert!(a.take_flag(&short('v')));
let w = a.take_positional_word(M).unwrap().unwrap();
let w = a.take_positional_word(M).unwrap();
assert_eq!(w.2, "-x");
assert!(a.is_empty());
}
Expand All @@ -874,7 +871,7 @@ mod tests {
fn positionals_after_double_dash2() {
let mut a = State::from(&["-v", "--", "-x"]);
assert!(a.take_flag(&short('v')));
let w = a.take_positional_word(M).unwrap().unwrap();
let w = a.take_positional_word(M).unwrap();
assert_eq!(w.2, "-x");
assert!(a.is_empty());
}
Expand All @@ -884,7 +881,7 @@ mod tests {
let mut a = State::from(&["-v", "12", "--", "-x"]);
let w = a.take_arg(&short('v'), false, M).unwrap().unwrap();
assert_eq!(w, "12");
let w = a.take_positional_word(M).unwrap().unwrap();
let w = a.take_positional_word(M).unwrap();
assert_eq!(w.2, "-x");
assert!(a.is_empty());
}
Expand Down
2 changes: 1 addition & 1 deletion src/docs2/adjacent_struct_0.md
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ Options { point: [Point { point: (), x: 10, y: 20, z: 3.1415 }, Point { point: (

<div class='bpaf-doc'>
$ app --point 10 20 --rotate 3.1415<br>
<b>Error:</b> expected <tt><i>Z</i></tt>, got <b>--rotate</b>. Pass <tt><b>--help</b></tt> for usage information
<b>Error:</b> expected <tt><i>Z</i></tt>, pass <tt><b>--help</b></tt> for usage information
<style>
div.bpaf-doc {
padding: 14px;
Expand Down
2 changes: 1 addition & 1 deletion src/docs2/compose_basic_to_options.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ The other one is `--version` - passing a string literal or something like

<div class='bpaf-doc'>
$ app --version<br>
Version: 3.1415
<p>Version: 3.1415</p>
<style>
div.bpaf-doc {
padding: 14px;
Expand Down
4 changes: 2 additions & 2 deletions src/docs2/custom_help_version.md
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ both working

<div class='bpaf-doc'>
$ app --version<br>
Version: 0.42
<p>Version: 0.42</p>
<style>
div.bpaf-doc {
padding: 14px;
Expand All @@ -134,7 +134,7 @@ div.bpaf-doc { padding-left: 1em; }

<div class='bpaf-doc'>
$ app -v<br>
Version: 0.42
<p>Version: 0.42</p>
<style>
div.bpaf-doc {
padding: 14px;
Expand Down
2 changes: 1 addition & 1 deletion src/docs2/derive_basic_custom_consumer.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ long name is missing

<div class='bpaf-doc'>
$ app --switch 42<br>
<b>Error:</b> expected <tt><i>NUM</i></tt>, got <b>--switch</b>. Pass <tt><b>--help</b></tt> for usage information
<b>Error:</b> <b>--switch</b> is not expected in this context
<style>
div.bpaf-doc {
padding: 14px;
Expand Down
2 changes: 1 addition & 1 deletion src/docs2/to_options.md
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ The other one is `--version` - passing a string literal or something like

<div class='bpaf-doc'>
$ app --version<br>
Version: 3.1415
<p>Version: 3.1415</p>
<style>
div.bpaf-doc {
padding: 14px;
Expand Down
47 changes: 20 additions & 27 deletions src/params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -794,36 +794,29 @@ fn parse_pos_word(
help: &Option<Doc>,
) -> Result<OsString, Error> {
let metavar = Metavar(metavar);
if let Some((ix, is_strict, word)) = args.take_positional_word(metavar)? {
if strict && !is_strict {
#[cfg(feature = "autocomplete")]
args.push_pos_sep();
match args.take_positional_word(metavar) {
Ok((ix, is_strict, word)) => {
if strict && !is_strict {
#[cfg(feature = "autocomplete")]
args.push_pos_sep();

return Err(Error(Message::StrictPos(ix, metavar)));
}
#[cfg(feature = "autocomplete")]
if args.touching_last_remove() && !args.check_no_pos_ahead() {
args.push_metavar(metavar.0, help, false);
args.set_no_pos_ahead();
return Err(Error(Message::StrictPos(ix, metavar)));
}
#[cfg(feature = "autocomplete")]
if args.touching_last_remove() && !args.check_no_pos_ahead() {
args.push_metavar(metavar.0, help, false);
args.set_no_pos_ahead();
}
Ok(word)
}
Ok(word)
} else {
#[cfg(feature = "autocomplete")]
if !args.check_no_pos_ahead() {
args.push_metavar(metavar.0, help, false);
args.set_no_pos_ahead();
Err(err) => {
#[cfg(feature = "autocomplete")]
if !args.check_no_pos_ahead() {
args.push_metavar(metavar.0, help, false);
args.set_no_pos_ahead();
}
Err(err)
}

let position = args.items_iter().next().map_or(args.scope().end, |x| x.0);
let missing = MissingItem {
item: Item::Positional {
metavar,
help: help.clone(),
},
position,
scope: args.scope(),
};
Err(Error(Message::Missing(vec![missing])))
}
}

Expand Down
32 changes: 32 additions & 0 deletions tests/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -199,3 +199,35 @@ fn adjacent_option_complains_to() {
// TODO - this should point to the whole "-ayam" thing
assert_eq!(r, "couldn't parse `yam`: invalid digit found in string");
}

#[test]
fn some_pos_with_invalid_flag() {
let a = short('a').switch();
let b = positional::<usize>("B").some("Want B");
let parser = construct!(a, b).to_options();

let r = parser.run_inner(&["-c", "12"]).unwrap_err().unwrap_stderr();
assert_eq!(r, "`-c` is not expected in this context");

let r = parser.run_inner(&["12", "-c"]).unwrap_err().unwrap_stderr();
assert_eq!(r, "`-c` is not expected in this context");
}

#[test]
fn pos_with_invalid_arg() {
let a = short('a').argument::<usize>("A").optional();
let b = positional::<usize>("B");
let parser = construct!(a, b).to_options();

let r = parser.run_inner(&["-c", "12"]).unwrap_err().unwrap_stderr();
assert_eq!(r, "`-c` is not expected in this context");

let r = parser.run_inner(&["12", "-c"]).unwrap_err().unwrap_stderr();
assert_eq!(r, "`-c` is not expected in this context");

let r = parser.run_inner(&["-c", "t"]).unwrap_err().unwrap_stderr();
assert_eq!(r, "couldn't parse `t`: invalid digit found in string");

let r = parser.run_inner(&["t", "-c"]).unwrap_err().unwrap_stderr();
assert_eq!(r, "couldn't parse `t`: invalid digit found in string");
}

0 comments on commit c919d73

Please sign in to comment.