Repository: google/argh
Pull request #147 (2022-10-28 01:54:32)

Allow `#[argh(switch)]` on Option<bool> fields
It can be useful to use Option<bool> in a configuration struct, in particular if that struct can be built from another source (e.g. deserialized from a configuration file) where boolean switches can be explicitly set to false and we want to differenciate between the flag being negated explicitly from it not being specified.

Thus, allow the `switch` parameters to be used on Option<bool> fields. The field will be `None` if the switch was not specified, and `Some(true)` if it was.


Patch:
diff --git a/argh/src/lib.rs b/argh/src/lib.rs
index 9e955da..bd83cb4 100644
--- a/argh/src/lib.rs
+++ b/argh/src/lib.rs
@@ -822,6 +822,16 @@ impl Flag for bool {
     }
 }
 
+impl Flag for Option<bool> {
+    fn default() -> Self {
+        None
+    }
+
+    fn set_flag(&mut self) {
+        *self = Some(true);
+    }
+}
+
 macro_rules! impl_flag_for_integers {
     ($($ty:ty,)*) => {
         $(
diff --git a/argh_derive/src/help.rs b/argh_derive/src/help.rs
index 9f8bbe7..bd20622 100644
--- a/argh_derive/src/help.rs
+++ b/argh_derive/src/help.rs
@@ -27,7 +27,6 @@ pub(crate) fn help(
     fields: &[StructField<'_>],
     subcommand: Option<&StructField<'_>>,
 ) -> TokenStream {
-    #![allow(clippy::format_push_string)]
     let mut format_lit = "Usage: {command_name}".to_string();
 
     let positional = fields.iter().filter(|f| {
@@ -156,7 +155,7 @@ fn positional_usage(out: &mut String, field: &StructField<'_>) {
     if field.attrs.greedy.is_none() {
         out.push('<');
     }
-    let name = field.arg_name();
+    let name = field.positional_arg_name();
     out.push_str(&name);
     if field.optionality == Optionality::Repeating {
         out.push_str("...");
@@ -231,7 +230,7 @@ Add a doc comment or an `#[argh(description = \"...\")]` attribute.",
 /// Describes a positional argument like this:
 ///  hello       positional argument description
 fn positional_description(out: &mut String, field: &StructField<'_>) {
-    let field_name = field.arg_name();
+    let field_name = field.positional_arg_name();
 
     let mut description = String::from("");
     if let Some(desc) = &field.attrs.description {
diff --git a/argh_derive/src/lib.rs b/argh_derive/src/lib.rs
index 49a6c17..d790b06 100644
--- a/argh_derive/src/lib.rs
+++ b/argh_derive/src/lib.rs
@@ -17,7 +17,7 @@ use {
     proc_macro2::{Span, TokenStream},
     quote::{quote, quote_spanned, ToTokens},
     std::{collections::HashMap, str::FromStr},
-    syn::{spanned::Spanned, LitStr},
+    syn::{spanned::Spanned, GenericArgument, LitStr, PathArguments, Type},
 };
 
 mod errors;
@@ -196,8 +196,12 @@ impl<'a> StructField<'a> {
         Some(StructField { field, attrs, kind, optionality, ty_without_wrapper, name, long_name })
     }
 
-    pub(crate) fn arg_name(&self) -> String {
-        self.attrs.arg_name.as_ref().map(LitStr::value).unwrap_or_else(|| self.name.to_string())
+    pub(crate) fn positional_arg_name(&self) -> String {
+        self.attrs
+            .arg_name
+            .as_ref()
+            .map(LitStr::value)
+            .unwrap_or_else(|| self.name.to_string().trim_matches('_').to_owned())
     }
 }
 
@@ -530,7 +534,7 @@ fn ensure_unique_names(errors: &Errors, fields: &[StructField<'_>]) {
                     first_use_field,
                     &format!("The short name of \"-{}\" was already used here.", short_name),
                 );
-                errors.err_span_tokens(&field.field, "Later usage here.");
+                errors.err_span_tokens(field.field, "Later usage here.");
             }
 
             seen_short_names.insert(short_name, &field.field);
@@ -542,7 +546,7 @@ fn ensure_unique_names(errors: &Errors, fields: &[StructField<'_>]) {
                     *first_use_field,
                     &format!("The long name of \"{}\" was already used here.", long_name),
                 );
-                errors.err_span_tokens(&field.field, "Later usage here.");
+                errors.err_span_tokens(field.field, "Later usage here.");
             }
 
             seen_long_names.insert(long_name, field.field);
@@ -710,7 +714,7 @@ fn declare_local_storage_for_redacted_fields<'a>(
                     }
                 };
 
-                let arg_name = field.arg_name();
+                let arg_name = field.positional_arg_name();
                 quote! {
                     let mut #field_name: argh::ParseValueSlotTy::<#field_slot_type, String> =
                         argh::ParseValueSlotTy {
@@ -802,7 +806,7 @@ fn append_missing_requirements<'a>(
         match field.kind {
             FieldKind::Switch => unreachable!("switches are always optional"),
             FieldKind::Positional => {
-                let name = field.arg_name();
+                let name = field.positional_arg_name();
                 quote! {
                     if #field_name.slot.is_none() {
                         #mri.missing_positional_arg(#name)
@@ -850,6 +854,16 @@ fn ty_expect_switch(errors: &Errors, ty: &syn::Type) -> bool {
                 return false;
             }
             let ident = &path.path.segments[0].ident;
+            // `Option<bool>` can be used as a `switch`.
+            if ident == "Option" {
+                if let PathArguments::AngleBracketed(args) = &path.path.segments[0].arguments {
+                    if let GenericArgument::Type(Type::Path(p)) = &args.args[0] {
+                        if p.path.segments[0].ident == "bool" {
+                            return true;
+                        }
+                    }
+                }
+            }
             ["bool", "u8", "u16", "u32", "u64", "u128", "i8", "i16", "i32", "i64", "i128"]
                 .iter()
                 .any(|path| ident == path)
@@ -860,7 +874,7 @@ fn ty_expect_switch(errors: &Errors, ty: &syn::Type) -> bool {
 
     let res = ty_can_be_switch(ty);
     if !res {
-        errors.err(ty, "switches must be of type `bool` or integer type");
+        errors.err(ty, "switches must be of type `bool`, `Option<bool>`, or integer type");
     }
     res
 }


Test patch:
diff --git a/argh/tests/lib.rs b/argh/tests/lib.rs
index 522a513..f8068ff 100644
--- a/argh/tests/lib.rs
+++ b/argh/tests/lib.rs
@@ -703,6 +703,26 @@ Required options not provided:
             },
         );
     }
+
+    #[derive(FromArgs, Debug, PartialEq)]
+    /// Woot
+    struct Underscores {
+        #[argh(positional)]
+        /// fooey
+        a_: String,
+    }
+
+    #[test]
+    fn positional_name_with_underscores() {
+        assert_output(&["first"], Underscores { a_: "first".into() });
+
+        assert_error::<Underscores>(
+            &[],
+            r###"Required positional arguments not provided:
+    a
+"###,
+        );
+    }
 }
 
 /// Tests derived from