From 2e3540355ac3c2d641e76a913c64af5cf78c3f3f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 9 May 2022 10:35:52 -0500 Subject: [PATCH] fix(derive): Don't change case of Arg id's (unstable) This will make it easier to reference arguments with different attributes. Fixes #3282 --- clap_derive/src/attrs.rs | 21 ++++ clap_derive/src/derives/args.rs | 38 ++++---- tests/derive/help.rs | 165 +++++++++++++++++++++++--------- tests/derive/naming.rs | 10 +- 4 files changed, 165 insertions(+), 69 deletions(-) diff --git a/clap_derive/src/attrs.rs b/clap_derive/src/attrs.rs index 9a7bb7e7b29..aaee167e31c 100644 --- a/clap_derive/src/attrs.rs +++ b/clap_derive/src/attrs.rs @@ -671,6 +671,16 @@ impl Attrs { quote!( #(#next_help_heading)* #(#help_heading)* ) } + #[cfg(feature = "unstable-v4")] + pub fn id(&self) -> TokenStream { + self.name.clone().raw() + } + + #[cfg(not(feature = "unstable-v4"))] + pub fn id(&self) -> TokenStream { + self.cased_name() + } + pub fn cased_name(&self) -> TokenStream { self.name.clone().translate(*self.casing) } @@ -916,6 +926,17 @@ pub enum Name { } impl Name { + #[cfg(feature = "unstable-v4")] + pub fn raw(self) -> TokenStream { + match self { + Name::Assigned(tokens) => tokens, + Name::Derived(ident) => { + let s = ident.unraw().to_string(); + quote_spanned!(ident.span()=> #s) + } + } + } + pub fn translate(self, style: CasingStyle) -> TokenStream { use CasingStyle::*; diff --git a/clap_derive/src/derives/args.rs b/clap_derive/src/derives/args.rs index 077fe4a64dd..e1486ea2bd3 100644 --- a/clap_derive/src/derives/args.rs +++ b/clap_derive/src/derives/args.rs @@ -342,12 +342,12 @@ pub fn gen_augment( } }; - let name = attrs.cased_name(); + let id = attrs.id(); let methods = attrs.field_methods(true); Some(quote_spanned! { field.span()=> let #app_var = #app_var.arg( - clap::Arg::new(#name) + clap::Arg::new(#id) #modifier #methods ); @@ -528,7 +528,7 @@ fn gen_parsers( let func = &parser.func; let span = parser.kind.span(); let convert_type = inner_type(**ty, &field.ty); - let name = attrs.cased_name(); + let id = attrs.id(); let (value_of, values_of, mut parse) = match *parser.kind { FromStr => ( quote_spanned!(span=> value_of), @@ -538,7 +538,7 @@ fn gen_parsers( TryFromStr => ( quote_spanned!(span=> value_of), quote_spanned!(span=> values_of), - quote_spanned!(func.span()=> |s| #func(s).map_err(|err| clap::Error::raw(clap::ErrorKind::ValueValidation, format!("Invalid value for {}: {}", #name, err)))), + quote_spanned!(func.span()=> |s| #func(s).map_err(|err| clap::Error::raw(clap::ErrorKind::ValueValidation, format!("Invalid value for {}: {}", #id, err)))), ), FromOsStr => ( quote_spanned!(span=> value_of_os), @@ -548,7 +548,7 @@ fn gen_parsers( TryFromOsStr => ( quote_spanned!(span=> value_of_os), quote_spanned!(span=> values_of_os), - quote_spanned!(func.span()=> |s| #func(s).map_err(|err| clap::Error::raw(clap::ErrorKind::ValueValidation, format!("Invalid value for {}: {}", #name, err)))), + quote_spanned!(func.span()=> |s| #func(s).map_err(|err| clap::Error::raw(clap::ErrorKind::ValueValidation, format!("Invalid value for {}: {}", #id, err)))), ), FromOccurrences => ( quote_spanned!(span=> occurrences_of), @@ -561,7 +561,7 @@ fn gen_parsers( let ci = attrs.ignore_case(); parse = quote_spanned! { convert_type.span()=> - |s| <#convert_type as clap::ArgEnum>::from_str(s, #ci).map_err(|err| clap::Error::raw(clap::ErrorKind::ValueValidation, format!("Invalid value for {}: {}", #name, err))) + |s| <#convert_type as clap::ArgEnum>::from_str(s, #ci).map_err(|err| clap::Error::raw(clap::ErrorKind::ValueValidation, format!("Invalid value for {}: {}", #id, err))) } } @@ -576,34 +576,34 @@ fn gen_parsers( Ty::Bool => { if update.is_some() { quote_spanned! { ty.span()=> - *#field_name || #arg_matches.is_present(#name) + *#field_name || #arg_matches.is_present(#id) } } else { quote_spanned! { ty.span()=> - #arg_matches.is_present(#name) + #arg_matches.is_present(#id) } } } Ty::Option => { quote_spanned! { ty.span()=> - #arg_matches.#value_of(#name) + #arg_matches.#value_of(#id) .map(#parse) .transpose()? } } Ty::OptionOption => quote_spanned! { ty.span()=> - if #arg_matches.is_present(#name) { - Some(#arg_matches.#value_of(#name).map(#parse).transpose()?) + if #arg_matches.is_present(#id) { + Some(#arg_matches.#value_of(#id).map(#parse).transpose()?) } else { None } }, Ty::OptionVec => quote_spanned! { ty.span()=> - if #arg_matches.is_present(#name) { - Some(#arg_matches.#values_of(#name) + if #arg_matches.is_present(#id) { + Some(#arg_matches.#values_of(#id) .map(|v| v.map::<::std::result::Result<#convert_type, clap::Error>, _>(#parse).collect::<::std::result::Result, clap::Error>>()) .transpose()? .unwrap_or_else(Vec::new)) @@ -614,7 +614,7 @@ fn gen_parsers( Ty::Vec => { quote_spanned! { ty.span()=> - #arg_matches.#values_of(#name) + #arg_matches.#values_of(#id) .map(|v| v.map::<::std::result::Result<#convert_type, clap::Error>, _>(#parse).collect::<::std::result::Result, clap::Error>>()) .transpose()? .unwrap_or_else(Vec::new) @@ -622,17 +622,17 @@ fn gen_parsers( } Ty::Other if occurrences => quote_spanned! { ty.span()=> - #parse(#arg_matches.#value_of(#name)) + #parse(#arg_matches.#value_of(#id)) }, Ty::Other if flag => quote_spanned! { ty.span()=> - #parse(#arg_matches.is_present(#name)) + #parse(#arg_matches.is_present(#id)) }, Ty::Other => { quote_spanned! { ty.span()=> - #arg_matches.#value_of(#name) - .ok_or_else(|| clap::Error::raw(clap::ErrorKind::MissingRequiredArgument, format!("The following required argument was not provided: {}", #name))) + #arg_matches.#value_of(#id) + .ok_or_else(|| clap::Error::raw(clap::ErrorKind::MissingRequiredArgument, format!("The following required argument was not provided: {}", #id))) .and_then(#parse)? } } @@ -640,7 +640,7 @@ fn gen_parsers( if let Some(access) = update { quote_spanned! { field.span()=> - if #arg_matches.is_present(#name) { + if #arg_matches.is_present(#id) { #access *#field_name = #field_value } diff --git a/tests/derive/help.rs b/tests/derive/help.rs index 4cd1d943c9e..1362ac866c8 100644 --- a/tests/derive/help.rs +++ b/tests/derive/help.rs @@ -14,16 +14,26 @@ fn arg_help_heading_applied() { let cmd = CliOptions::command(); - let should_be_in_section_a = cmd - .get_arguments() - .find(|a| a.get_id() == "should-be-in-section-a") - .unwrap(); + let should_be_in_section_a = if cfg!(feature = "unstable-v4") { + cmd.get_arguments() + .find(|a| a.get_id() == "should_be_in_section_a") + .unwrap() + } else { + cmd.get_arguments() + .find(|a| a.get_id() == "should-be-in-section-a") + .unwrap() + }; assert_eq!(should_be_in_section_a.get_help_heading(), Some("HEADING A")); - let should_be_in_section_b = cmd - .get_arguments() - .find(|a| a.get_id() == "no-section") - .unwrap(); + let should_be_in_section_b = if cfg!(feature = "unstable-v4") { + cmd.get_arguments() + .find(|a| a.get_id() == "no_section") + .unwrap() + } else { + cmd.get_arguments() + .find(|a| a.get_id() == "no-section") + .unwrap() + }; assert_eq!(should_be_in_section_b.get_help_heading(), None); } @@ -42,16 +52,26 @@ fn app_help_heading_applied() { let cmd = CliOptions::command(); - let should_be_in_section_a = cmd - .get_arguments() - .find(|a| a.get_id() == "should-be-in-section-a") - .unwrap(); + let should_be_in_section_a = if cfg!(feature = "unstable-v4") { + cmd.get_arguments() + .find(|a| a.get_id() == "should_be_in_section_a") + .unwrap() + } else { + cmd.get_arguments() + .find(|a| a.get_id() == "should-be-in-section-a") + .unwrap() + }; assert_eq!(should_be_in_section_a.get_help_heading(), Some("HEADING A")); - let should_be_in_default_section = cmd - .get_arguments() - .find(|a| a.get_id() == "should-be-in-default-section") - .unwrap(); + let should_be_in_default_section = if cfg!(feature = "unstable-v4") { + cmd.get_arguments() + .find(|a| a.get_id() == "should_be_in_default_section") + .unwrap() + } else { + cmd.get_arguments() + .find(|a| a.get_id() == "should-be-in-default-section") + .unwrap() + }; assert_eq!( should_be_in_default_section.get_help_heading(), Some("DEFAULT") @@ -119,47 +139,83 @@ fn app_help_heading_flattened() { let cmd = CliOptions::command(); - let should_be_in_section_a = cmd - .get_arguments() - .find(|a| a.get_id() == "should-be-in-section-a") - .unwrap(); + let should_be_in_section_a = if cfg!(feature = "unstable-v4") { + cmd.get_arguments() + .find(|a| a.get_id() == "should_be_in_section_a") + .unwrap() + } else { + cmd.get_arguments() + .find(|a| a.get_id() == "should-be-in-section-a") + .unwrap() + }; assert_eq!(should_be_in_section_a.get_help_heading(), Some("HEADING A")); - let should_be_in_section_b = cmd - .get_arguments() - .find(|a| a.get_id() == "should-be-in-section-b") - .unwrap(); + let should_be_in_section_b = if cfg!(feature = "unstable-v4") { + cmd.get_arguments() + .find(|a| a.get_id() == "should_be_in_section_b") + .unwrap() + } else { + cmd.get_arguments() + .find(|a| a.get_id() == "should-be-in-section-b") + .unwrap() + }; assert_eq!(should_be_in_section_b.get_help_heading(), Some("HEADING B")); - let should_be_in_default_section = cmd - .get_arguments() - .find(|a| a.get_id() == "should-be-in-default-section") - .unwrap(); + let should_be_in_default_section = if cfg!(feature = "unstable-v4") { + cmd.get_arguments() + .find(|a| a.get_id() == "should_be_in_default_section") + .unwrap() + } else { + cmd.get_arguments() + .find(|a| a.get_id() == "should-be-in-default-section") + .unwrap() + }; assert_eq!(should_be_in_default_section.get_help_heading(), None); let sub_a_two = cmd.find_subcommand("sub-a-two").unwrap(); - let should_be_in_sub_a = sub_a_two - .get_arguments() - .find(|a| a.get_id() == "should-be-in-sub-a") - .unwrap(); + let should_be_in_sub_a = if cfg!(feature = "unstable-v4") { + sub_a_two + .get_arguments() + .find(|a| a.get_id() == "should_be_in_sub_a") + .unwrap() + } else { + sub_a_two + .get_arguments() + .find(|a| a.get_id() == "should-be-in-sub-a") + .unwrap() + }; assert_eq!(should_be_in_sub_a.get_help_heading(), Some("SUB A")); let sub_b_one = cmd.find_subcommand("sub-b-one").unwrap(); - let should_be_in_sub_b = sub_b_one - .get_arguments() - .find(|a| a.get_id() == "should-be-in-sub-b") - .unwrap(); + let should_be_in_sub_b = if cfg!(feature = "unstable-v4") { + sub_b_one + .get_arguments() + .find(|a| a.get_id() == "should_be_in_sub_b") + .unwrap() + } else { + sub_b_one + .get_arguments() + .find(|a| a.get_id() == "should-be-in-sub-b") + .unwrap() + }; assert_eq!(should_be_in_sub_b.get_help_heading(), Some("SUB B")); let sub_c = cmd.find_subcommand("sub-c").unwrap(); let sub_c_one = sub_c.find_subcommand("sub-c-one").unwrap(); - let should_be_in_sub_c = sub_c_one - .get_arguments() - .find(|a| a.get_id() == "should-be-in-sub-c") - .unwrap(); + let should_be_in_sub_c = if cfg!(feature = "unstable-v4") { + sub_c_one + .get_arguments() + .find(|a| a.get_id() == "should_be_in_sub_c") + .unwrap() + } else { + sub_c_one + .get_arguments() + .find(|a| a.get_id() == "should-be-in-sub-c") + .unwrap() + }; assert_eq!(should_be_in_sub_c.get_help_heading(), Some("SUB C")); } @@ -180,10 +236,15 @@ fn flatten_field_with_help_heading() { let cmd = CliOptions::command(); - let should_be_in_section_a = cmd - .get_arguments() - .find(|a| a.get_id() == "should-be-in-section-a") - .unwrap(); + let should_be_in_section_a = if cfg!(feature = "unstable-v4") { + cmd.get_arguments() + .find(|a| a.get_id() == "should_be_in_section_a") + .unwrap() + } else { + cmd.get_arguments() + .find(|a| a.get_id() == "should-be-in-section-a") + .unwrap() + }; assert_eq!(should_be_in_section_a.get_help_heading(), Some("HEADING A")); } @@ -218,7 +279,8 @@ fn derive_generated_error_has_full_context() { result.unwrap() ); - let expected = r#"error: The following required argument was not provided: req-str + if cfg!(feature = "unstable-v4") { + let expected = r#"error: The following required argument was not provided: req_str USAGE: clap --req-str @@ -226,7 +288,18 @@ USAGE: For more information try --help "#; - assert_eq!(result.unwrap_err().to_string(), expected); + assert_eq!(result.unwrap_err().to_string(), expected); + } else { + let expected = r#"error: The following required argument was not provided: req-str + +USAGE: + clap --req-str + clap + +For more information try --help +"#; + assert_eq!(result.unwrap_err().to_string(), expected); + } } #[test] diff --git a/tests/derive/naming.rs b/tests/derive/naming.rs index 0ef656ebc66..e3018a02f7c 100644 --- a/tests/derive/naming.rs +++ b/tests/derive/naming.rs @@ -296,7 +296,7 @@ fn test_rename_all_is_propagated_from_enum_to_variants() { FirstVariant, SecondVariant { #[clap(long)] - foo: bool, + foo: String, }, } @@ -314,13 +314,15 @@ fn test_rename_all_is_propagated_from_enum_to_variant_fields() { FirstVariant, SecondVariant { #[clap(long)] - foo: bool, + foo: String, }, } assert_eq!( - Opt::SecondVariant { foo: true }, - Opt::try_parse_from(&["test", "SECOND_VARIANT", "--FOO"]).unwrap() + Opt::SecondVariant { + foo: "value".into() + }, + Opt::try_parse_from(&["test", "SECOND_VARIANT", "--FOO", "value"]).unwrap() ); }