Repository: pacak/bpaf
Pull request #285 (2023-08-28 17:45:50)

Make sure hidden env-only flags, switches and arguments work
Removes one of the `unreachable!()` branches that became reachable after I added `env` only parsers. Parse, don't validate yet again...


Patch:
diff --git a/Changelog.md b/Changelog.md
index 5e0c00a..6a5014e 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -1,5 +1,8 @@
 # Change Log
 
+## bpaf [0.9.6], bpaf_derive [0.5.6] - Unreleased
+- make sure env-only arguments and flags are working
+
 ## 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
 - `bpaf_derive`: make sure command aliases are actually working
diff --git a/src/complete_gen.rs b/src/complete_gen.rs
index b90258b..ee2896b 100644
--- a/src/complete_gen.rs
+++ b/src/complete_gen.rs
@@ -49,14 +49,16 @@ impl State {
     pub(crate) fn push_flag(&mut self, named: &NamedArg) {
         let depth = self.depth();
         if let Some(comp) = self.comp_mut() {
-            comp.comps.push(Comp::Flag {
-                extra: CompExtra {
-                    depth,
-                    group: None,
-                    help: named.help.as_ref().and_then(Doc::to_completion),
-                },
-                name: ShortLong::from(named),
-            });
+            if let Ok(name) = ShortLong::try_from(named) {
+                comp.comps.push(Comp::Flag {
+                    extra: CompExtra {
+                        depth,
+                        group: None,
+                        help: named.help.as_ref().and_then(Doc::to_completion),
+                    },
+                    name,
+                });
+            }
         }
     }
 
@@ -64,15 +66,17 @@ impl State {
     pub(crate) fn push_argument(&mut self, named: &NamedArg, metavar: &'static str) {
         let depth = self.depth();
         if let Some(comp) = self.comp_mut() {
-            comp.comps.push(Comp::Argument {
-                extra: CompExtra {
-                    depth,
-                    group: None,
-                    help: named.help.as_ref().and_then(Doc::to_completion),
-                },
-                metavar,
-                name: ShortLong::from(named),
-            });
+            if let Ok(name) = ShortLong::try_from(named) {
+                comp.comps.push(Comp::Argument {
+                    extra: CompExtra {
+                        depth,
+                        group: None,
+                        help: named.help.as_ref().and_then(Doc::to_completion),
+                    },
+                    metavar,
+                    name,
+                });
+            }
         }
     }
 
diff --git a/src/item.rs b/src/item.rs
index 1e4f7e7..f65e779 100644
--- a/src/item.rs
+++ b/src/item.rs
@@ -116,13 +116,15 @@ impl ShortLong {
     }
 }
 
-impl From<&NamedArg> for ShortLong {
-    fn from(named: &NamedArg) -> Self {
+impl TryFrom<&NamedArg> for ShortLong {
+    type Error = ();
+
+    fn try_from(named: &NamedArg) -> Result<Self, Self::Error> {
         match (named.short.is_empty(), named.long.is_empty()) {
-            (true, true) => unreachable!("Named should have either short or long name"),
-            (true, false) => Self::Long(named.long[0]),
-            (false, true) => Self::Short(named.short[0]),
-            (false, false) => Self::ShortLong(named.short[0], named.long[0]),
+            (true, true) => Err(()),
+            (true, false) => Ok(Self::Long(named.long[0])),
+            (false, true) => Ok(Self::Short(named.short[0])),
+            (false, false) => Ok(Self::ShortLong(named.short[0], named.long[0])),
         }
     }
 }
diff --git a/src/params.rs b/src/params.rs
index fd1682a..ea45940 100644
--- a/src/params.rs
+++ b/src/params.rs
@@ -112,13 +112,13 @@ pub struct NamedArg {
 }
 
 impl NamedArg {
-    pub(crate) fn flag_item(&self) -> Item {
-        Item::Flag {
-            name: ShortLong::from(self),
+    pub(crate) fn flag_item(&self) -> Option<Item> {
+        Some(Item::Flag {
+            name: ShortLong::try_from(self).ok()?,
             help: self.help.clone(),
             env: self.env.first().copied(),
             shorts: self.short.clone(),
-        }
+        })
     }
 }
 
@@ -546,19 +546,29 @@ impl<T: Clone + 'static> Parser<T> for ParseFlag<T> {
             match &self.absent {
                 Some(ok) => Ok(ok.clone()),
                 None => {
-                    let missing = MissingItem {
-                        item: self.named.flag_item(),
-                        position: args.scope().start,
-                        scope: args.scope(),
-                    };
-                    Err(Error(Message::Missing(vec![missing])))
+                    if let Some(item) = self.named.flag_item() {
+                        let missing = MissingItem {
+                            item,
+                            position: args.scope().start,
+                            scope: args.scope(),
+                        };
+                        Err(Error(Message::Missing(vec![missing])))
+                    } else if let Some(name) = self.named.env.first() {
+                        Err(Error(Message::NoEnv(name)))
+                    } else {
+                        todo!("no key!")
+                    }
                 }
             }
         }
     }
 
     fn meta(&self) -> Meta {
-        self.named.flag_item().required(self.absent.is_none())
+        if let Some(item) = self.named.flag_item() {
+            item.required(self.absent.is_none())
+        } else {
+            Meta::Skip
+        }
     }
 }
 
@@ -624,22 +634,17 @@ impl<T> ParseArgument<T> {
         self
     }
 
-    fn item(&self) -> Item {
-        Item::Argument {
-            name: ShortLong::from(&self.named),
+    fn item(&self) -> Option<Item> {
+        Some(Item::Argument {
+            name: ShortLong::try_from(&self.named).ok()?,
             metavar: Metavar(self.metavar),
             env: self.named.env.first().copied(),
             help: self.named.help.clone(),
             shorts: self.named.short.clone(),
-        }
+        })
     }
 
     fn take_argument(&self, args: &mut State) -> Result<OsString, Error> {
-        if self.named.short.is_empty() && self.named.long.is_empty() {
-            if let Some(name) = self.named.env.first() {
-                return Err(Error(Message::NoEnv(name)));
-            }
-        }
         match args.take_arg(&self.named, self.adjacent, Metavar(self.metavar)) {
             Ok(Some(w)) => {
                 #[cfg(feature = "autocomplete")]
@@ -658,14 +663,20 @@ impl<T> ParseArgument<T> {
                 args.push_argument(&self.named, self.metavar);
                 if let Some(val) = self.named.env.iter().find_map(std::env::var_os) {
                     args.current = None;
-                    Ok(val)
-                } else {
+                    return Ok(val);
+                }
+
+                if let Some(item) = self.item() {
                     let missing = MissingItem {
-                        item: self.item(),
+                        item,
                         position: args.scope().start,
                         scope: args.scope(),
                     };
                     Err(Error(Message::Missing(vec![missing])))
+                } else if let Some(name) = self.named.env.first() {
+                    Err(Error(Message::NoEnv(name)))
+                } else {
+                    unreachable!()
                 }
             }
         }
@@ -686,10 +697,10 @@ where
     }
 
     fn meta(&self) -> Meta {
-        if self.named.short.is_empty() && self.named.long.is_empty() {
-            Meta::Skip
+        if let Some(item) = self.item() {
+            Meta::from(item)
         } else {
-            Meta::from(self.item())
+            Meta::Skip
         }
     }
 }


Test patch:
diff --git a/tests/tests.rs b/tests/tests.rs
index f4fc5ca..b97f2fe 100644
--- a/tests/tests.rs
+++ b/tests/tests.rs
@@ -1707,6 +1707,30 @@ fn many_env() {
     assert_eq!(r, vec!["top s3cr3t".to_owned()]);
 }
 
+#[test]
+fn env_hidden_arg() {
+    std::env::set_var("USER1", "top s3cr3t");
+    let parser = env("USER1").argument::<String>("USER").to_options();
+    let r = parser.run_inner(&[]).unwrap();
+    assert_eq!(r, "top s3cr3t");
+}
+
+#[test]
+fn env_hidden_switch() {
+    std::env::set_var("USER1", "top s3cr3t");
+    let parser = env("USER1").switch().to_options();
+    let r = parser.run_inner(&[]).unwrap();
+    assert!(r);
+}
+
+#[test]
+fn env_hidden_flag() {
+    std::env::set_var("USER1", "top s3cr3t");
+    let parser = env("USER1").flag(true, false).to_options();
+    let r = parser.run_inner(&[]).unwrap();
+    assert!(r);
+}
+
 #[test]
 fn some_env() {
     std::env::set_var("USER1", "top s3cr3t");