Repository: pacak/bpaf
Pull request #131 (2022-11-26 16:55:54)

improve errors when several conflicting options are present


Patch:
diff --git a/Changelog.md b/Changelog.md
index c99b00d..6c07cd7 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -1,5 +1,9 @@
 # Change Log
 
+## bpaf [0.7.5] - unreleased
+- improve error messages when several conflicting options are specified
+- improve category theory docs
+
 ## bpaf [0.7.4], bpaf_derive [0.3.3] - 2022-11-19
 - bpaf_derive: improve error message
 - bpaf: bugfix for bash static shell completion
diff --git a/src/args.rs b/src/args.rs
index a0e4a3e..95324e1 100644
--- a/src/args.rs
+++ b/src/args.rs
@@ -1,7 +1,25 @@
 use std::ffi::OsString;
 
 pub(crate) use crate::arg::*;
-use crate::{parsers::NamedArg, Error};
+use crate::{parsers::NamedArg, Error, Meta};
+
+/// Shows which branch of [`ParseOrElse`] parsed the argument
+#[derive(Debug, Clone)]
+pub(crate) enum Conflict {
+    /// Only one branch succeeded
+    Solo(Meta),
+    /// Both branches succeeded, first parser was taken in favor of the second one
+    Conflicts(Meta, Meta),
+}
+
+impl Conflict {
+    pub(crate) fn winner(&self) -> &Meta {
+        match self {
+            Conflict::Solo(s) => s,
+            Conflict::Conflicts(w, _) => w,
+        }
+    }
+}
 
 /// Hides [`Args`] internal implementation
 mod inner {
@@ -12,9 +30,9 @@ mod inner {
         rc::Rc,
     };
 
-    use crate::{Meta, ParseFailure};
+    use crate::ParseFailure;
 
-    use super::{push_vec, Arg};
+    use super::{push_vec, Arg, Conflict};
     /// All currently present command line parameters, use it for unit tests and manual parsing
     ///
     /// The easiest way to create `Args` is by using it's `From` instance.
@@ -61,7 +79,7 @@ mod inner {
 
         /// set of conflicts - usize contains the offset to the rejected item,
         /// first Meta contains accepted item, second meta contains rejected item
-        pub(crate) conflicts: BTreeMap<usize, (Meta, Meta)>,
+        pub(crate) conflicts: BTreeMap<usize, Conflict>,
     }
 
     impl<const N: usize> From<&[&str; N]> for Args {
diff --git a/src/info.rs b/src/info.rs
index 6a3412e..b1fbba9 100644
--- a/src/info.rs
+++ b/src/info.rs
@@ -4,8 +4,13 @@
 use std::marker::PhantomData;
 
 use crate::{
-    args::Args, item::Item, meta_help::render_help, meta_usage::to_usage_meta, params::short,
-    parsers::ParseCommand, Meta, ParseFailure, Parser,
+    args::{Args, Conflict},
+    item::Item,
+    meta_help::render_help,
+    meta_usage::to_usage_meta,
+    params::short,
+    parsers::ParseCommand,
+    Meta, ParseFailure, Parser,
 };
 
 /// Unsuccessful command line parsing outcome, internal representation
@@ -79,7 +84,7 @@ fn check_unexpected(args: &Args) -> Result<(), Error> {
         None => Ok(()),
         Some((ix, item)) => {
             let mut msg = format!("{} is not expected in this context", item);
-            if let Some((acc, rej)) = args.conflicts.get(&ix) {
+            if let Some(Conflict::Conflicts(acc, rej)) = args.conflicts.get(&ix) {
                 use std::fmt::Write;
                 write!(msg, ": {} cannot be used at the same time as {}", rej, acc).unwrap();
             }
diff --git a/src/structs.rs b/src/structs.rs
index 3ababa4..7dc1d6b 100644
--- a/src/structs.rs
+++ b/src/structs.rs
@@ -1,5 +1,5 @@
 //! Structures that implement different methods on [`Parser`] trait
-use crate::{info::Error, Args, Meta, Parser};
+use crate::{args::Conflict, info::Error, Args, Meta, Parser};
 use std::marker::PhantomData;
 
 #[cfg(feature = "autocomplete")]
@@ -203,14 +203,16 @@ where
             comp_items,
         )? {
             if res_b.is_some() {
-                args.conflicts
-                    .insert(args_b.head, (self.this.meta(), self.that.meta()));
+                remember_conflict(args, self.this.meta(), &mut args_b, self.that.meta());
+            } else {
+                remember_winner(args, self.this.meta());
             }
             Ok(res_a.unwrap())
         } else {
             if res_a.is_some() {
-                args.conflicts
-                    .insert(args_a.head, (self.that.meta(), self.this.meta()));
+                remember_conflict(args, self.that.meta(), &mut args_a, self.this.meta());
+            } else {
+                remember_winner(args, self.that.meta());
             }
             Ok(res_b.unwrap())
         }
@@ -221,6 +223,32 @@ where
     }
 }
 
+fn remember_winner(args: &mut Args, meta: Meta) {
+    args.conflicts
+        .entry(args.head)
+        .or_insert(Conflict::Solo(meta));
+}
+
+fn remember_conflict(args: &mut Args, winner: Meta, failed: &mut Args, loser: Meta) {
+    let winner = args
+        .conflicts
+        .entry(args.head)
+        .or_insert(Conflict::Solo(winner))
+        .winner()
+        .clone();
+
+    let loser = failed
+        .conflicts
+        .get(&failed.head)
+        .map(Conflict::winner)
+        .cloned()
+        .unwrap_or(loser);
+
+    args.conflicts
+        .entry(failed.head)
+        .or_insert(Conflict::Conflicts(winner, loser));
+}
+
 fn this_or_that_picks_first(
     err_a: Option<Error>,
     err_b: Option<Error>,


Test patch:
diff --git a/tests/errors.rs b/tests/errors.rs
index 1ef5476..ad7f55c 100644
--- a/tests/errors.rs
+++ b/tests/errors.rs
@@ -14,7 +14,6 @@ fn better_error_with_enum() {
         .run_inner(Args::from(&["--alpha", "--beta"]))
         .unwrap_err()
         .unwrap_stderr();
-
     assert_eq!(
         res,
         "--beta is not expected in this context: --beta cannot be used at the same time as --alpha"
@@ -24,13 +23,22 @@ fn better_error_with_enum() {
         .run_inner(Args::from(&["--alpha", "--gamma"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(res, "--gamma is not expected in this context: --gamma cannot be used at the same time as (--alpha | --beta)");
+    assert_eq!(res, "--gamma is not expected in this context: --gamma cannot be used at the same time as --alpha");
 
     let res = foo()
         .run_inner(Args::from(&["--beta", "--gamma"]))
         .unwrap_err()
         .unwrap_stderr();
-    assert_eq!(res, "--gamma is not expected in this context: --gamma cannot be used at the same time as (--alpha | --beta)");
+    assert_eq!(res, "--gamma is not expected in this context: --gamma cannot be used at the same time as --beta");
+
+    let res = foo()
+        .run_inner(Args::from(&["--alpha", "--beta", "--gamma"]))
+        .unwrap_err()
+        .unwrap_stderr();
+    assert_eq!(
+        res,
+        "--beta is not expected in this context: --beta cannot be used at the same time as --alpha"
+    );
 }
 
 #[test]
diff --git a/tests/tests.rs b/tests/tests.rs
index 12980ec..1ea695e 100644
--- a/tests/tests.rs
+++ b/tests/tests.rs
@@ -520,7 +520,7 @@ fn from_several_alternatives_pick_more_meaningful() {
         .unwrap_stderr();
     assert_eq!(
         err3,
-        "-a is not expected in this context: (-a | -b) cannot be used at the same time as -c"
+        "-a is not expected in this context: -a cannot be used at the same time as -c"
     );
 
     let err4 = parser
@@ -529,7 +529,7 @@ fn from_several_alternatives_pick_more_meaningful() {
         .unwrap_stderr();
     assert_eq!(
         err4,
-        "-c is not expected in this context: -c cannot be used at the same time as (-a | -b)"
+        "-c is not expected in this context: -c cannot be used at the same time as -a"
     );
 
     let err5 = parser
@@ -538,7 +538,7 @@ fn from_several_alternatives_pick_more_meaningful() {
         .unwrap_stderr();
     assert_eq!(
         err5,
-        "-b is not expected in this context: (-a | -b) cannot be used at the same time as -c"
+        "-b is not expected in this context: -b cannot be used at the same time as -c"
     );
 }