Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ArgValue derive #2762

Merged
merged 10 commits into from
Oct 1, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion clap_derive/examples/arg_enum.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,19 @@

use clap::{ArgEnum, Clap};

#[derive(ArgEnum, Debug, PartialEq)]
#[derive(ArgEnum, Debug, PartialEq, Clone)]
enum ArgChoice {
/// Descriptions are supported as doc-comment
Foo,
// Renames are supported
#[clap(name = "b-a-r")]
Bar,
// Aliases are supported
#[clap(alias = "b", alias = "z")]
Baz,
// Hiding variants from help and completion is supported
#[clap(hidden = true)]
Hidden,
}

#[derive(Clap, PartialEq, Debug)]
Expand Down
2 changes: 1 addition & 1 deletion clap_derive/examples/value_hints_derive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use std::ffi::OsString;
use std::io;
use std::path::PathBuf;

#[derive(ArgEnum, Debug, PartialEq)]
#[derive(ArgEnum, Debug, PartialEq, Clone)]
enum GeneratorChoice {
Bash,
Elvish,
Expand Down
8 changes: 0 additions & 8 deletions clap_derive/src/attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -819,14 +819,6 @@ impl Attrs {
}
}

pub fn enum_aliases(&self) -> Vec<TokenStream> {
self.methods
.iter()
.filter(|m| m.name == "alias")
.map(|m| m.args.clone())
.collect()
}

pub fn casing(&self) -> Sp<CasingStyle> {
self.casing.clone()
}
Expand Down
59 changes: 19 additions & 40 deletions clap_derive/src/derives/arg_enum.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,8 @@ pub fn gen_for_enum(name: &Ident, attrs: &[Attribute], e: &DataEnum) -> TokenStr
);

let lits = lits(&e.variants, &attrs);
let variants = gen_variants(&lits);
let from_str = gen_from_str(&lits);
let as_arg = gen_as_arg(&lits);
let arg_values = gen_arg_values(&lits);
let arg_value = gen_arg_value(&lits);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let arg_value = gen_arg_value(&lits);
let as_arg_value = gen_as_arg_value(&lits);

Once the trait method is renamed


quote! {
#[allow(dead_code, unreachable_code, unused_variables)]
Expand All @@ -65,9 +64,8 @@ pub fn gen_for_enum(name: &Ident, attrs: &[Attribute], e: &DataEnum) -> TokenStr
)]
#[deny(clippy::correctness)]
impl clap::ArgEnum for #name {
#variants
#from_str
#as_arg
#arg_values
#arg_value
}
}
}
Expand All @@ -87,54 +85,35 @@ fn lits(
if let Kind::Skip(_) = &*attrs.kind() {
None
} else {
Some((variant, attrs))
let fields = attrs.field_methods();
let name = attrs.cased_name();
Some((
quote! {
clap::ArgValue::new(#name)
#fields
},
variant.ident.clone(),
))
}
})
.flat_map(|(variant, attrs)| {
let mut ret = vec![(attrs.cased_name(), variant.ident.clone())];

attrs
.enum_aliases()
.into_iter()
.for_each(|x| ret.push((x, variant.ident.clone())));

ret
})
.collect::<Vec<_>>()
}

fn gen_variants(lits: &[(TokenStream, Ident)]) -> TokenStream {
let lit = lits.iter().map(|l| &l.0).collect::<Vec<_>>();

quote! {
const VARIANTS: &'static [&'static str] = &[#(#lit),*];
}
}

fn gen_from_str(lits: &[(TokenStream, Ident)]) -> TokenStream {
let (lit, variant): (Vec<TokenStream>, Vec<Ident>) = lits.iter().cloned().unzip();
fn gen_arg_values(lits: &[(TokenStream, Ident)]) -> TokenStream {
let lit = lits.iter().map(|l| &l.1).collect::<Vec<_>>();

quote! {
fn from_str(input: &str, case_insensitive: bool) -> ::std::result::Result<Self, String> {
let func = if case_insensitive {
::std::ascii::AsciiExt::eq_ignore_ascii_case
} else {
str::eq
};

match input {
#(val if func(val, #lit) => Ok(Self::#variant),)*
e => Err(format!("Invalid variant: {}", e)),
}
fn value_variants<'a>() -> &'a [Self]{
&[#(Self::#lit),*]
}
}
}

fn gen_as_arg(lits: &[(TokenStream, Ident)]) -> TokenStream {
fn gen_arg_value(lits: &[(TokenStream, Ident)]) -> TokenStream {
let (lit, variant): (Vec<TokenStream>, Vec<Ident>) = lits.iter().cloned().unzip();

quote! {
fn as_arg(&self) -> Option<&'static str> {
fn arg_value<'a>(&self) -> Option<clap::ArgValue<'a>> {
match self {
#(Self::#variant => Some(#lit),)*
_ => None
Expand Down
2 changes: 1 addition & 1 deletion clap_derive/src/derives/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ pub fn gen_augment(

fn gen_arg_enum_possible_values(ty: &Type) -> TokenStream {
quote_spanned! { ty.span()=>
.possible_values(<#ty as clap::ArgEnum>::VARIANTS.into_iter().copied())
.possible_values(<#ty as clap::ArgEnum>::value_variants().iter().filter_map(clap::ArgEnum::arg_value))
}
}

Expand Down
6 changes: 4 additions & 2 deletions clap_derive/src/dummies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,13 @@ pub fn args(name: &Ident) {
pub fn arg_enum(name: &Ident) {
append_dummy(quote! {
impl clap::ArgEnum for #name {
const VARIANTS: &'static [&'static str] = &[];
fn value_variants<'a>() -> &'a [Self]{
unimplemented!()
}
fn from_str(_input: &str, _case_insensitive: bool) -> Result<Self, String> {
unimplemented!()
}
fn as_arg(&self) -> Option<&'static str> {
fn arg_value<'a>(&self) -> Option<clap::ArgValue<'a>>{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about naming this as_arg_value?

Copy link
Member

@epage epage Sep 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The call might allocate (for aliases) and so as_ wouldn't work, though to_ would. I can go either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed it to to_arg_value now

unimplemented!()
}
}
Expand Down
41 changes: 24 additions & 17 deletions clap_derive/tests/arg_enum.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.
use clap::{ArgEnum, Clap};
use clap::{ArgEnum, ArgValue, Clap};

#[test]
fn basic() {
#[derive(ArgEnum, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug, Clone)]
enum ArgChoice {
Foo,
Bar,
Expand Down Expand Up @@ -40,7 +40,7 @@ fn basic() {

#[test]
fn default_value() {
#[derive(ArgEnum, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug, Clone)]
enum ArgChoice {
Foo,
Bar,
Expand All @@ -54,7 +54,7 @@ fn default_value() {

impl std::fmt::Display for ArgChoice {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
std::fmt::Display::fmt(self.as_arg().unwrap(), f)
std::fmt::Display::fmt(self.arg_value().unwrap().get_name(), f)
}
}

Expand Down Expand Up @@ -86,7 +86,7 @@ fn default_value() {

#[test]
fn multi_word_is_renamed_kebab() {
#[derive(ArgEnum, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug, Clone)]
#[allow(non_camel_case_types)]
enum ArgChoice {
FooBar,
Expand Down Expand Up @@ -116,7 +116,7 @@ fn multi_word_is_renamed_kebab() {

#[test]
fn variant_with_defined_casing() {
#[derive(ArgEnum, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug, Clone)]
enum ArgChoice {
#[clap(rename_all = "screaming_snake")]
FooBar,
Expand All @@ -139,7 +139,7 @@ fn variant_with_defined_casing() {

#[test]
fn casing_is_propogated_from_parent() {
#[derive(ArgEnum, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug, Clone)]
#[clap(rename_all = "screaming_snake")]
enum ArgChoice {
FooBar,
Expand All @@ -162,7 +162,7 @@ fn casing_is_propogated_from_parent() {

#[test]
fn casing_propogation_is_overridden() {
#[derive(ArgEnum, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug, Clone)]
#[clap(rename_all = "screaming_snake")]
enum ArgChoice {
#[clap(rename_all = "camel")]
Expand All @@ -187,7 +187,7 @@ fn casing_propogation_is_overridden() {

#[test]
fn case_insensitive() {
#[derive(ArgEnum, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug, Clone)]
enum ArgChoice {
Foo,
}
Expand All @@ -214,7 +214,7 @@ fn case_insensitive() {

#[test]
fn case_insensitive_set_to_false() {
#[derive(ArgEnum, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug, Clone)]
enum ArgChoice {
Foo,
}
Expand All @@ -236,7 +236,7 @@ fn case_insensitive_set_to_false() {

#[test]
fn alias() {
#[derive(ArgEnum, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug, Clone)]
enum ArgChoice {
#[clap(alias = "TOTP")]
TOTP,
Expand Down Expand Up @@ -264,7 +264,7 @@ fn alias() {

#[test]
fn multiple_alias() {
#[derive(ArgEnum, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug, Clone)]
enum ArgChoice {
#[clap(alias = "TOTP", alias = "t")]
TOTP,
Expand Down Expand Up @@ -298,7 +298,7 @@ fn multiple_alias() {

#[test]
fn option() {
#[derive(ArgEnum, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug, Clone)]
enum ArgChoice {
Foo,
Bar,
Expand Down Expand Up @@ -328,7 +328,7 @@ fn option() {

#[test]
fn vector() {
#[derive(ArgEnum, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug, Clone)]
enum ArgChoice {
Foo,
Bar,
Expand Down Expand Up @@ -358,7 +358,7 @@ fn vector() {

#[test]
fn skip_variant() {
#[derive(ArgEnum, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug, Clone)]
#[allow(dead_code)] // silence warning about `Baz` being unused
enum ArgChoice {
Foo,
Expand All @@ -367,15 +367,22 @@ fn skip_variant() {
Baz,
}

assert_eq!(ArgChoice::VARIANTS, ["foo", "bar"]);
assert_eq!(
ArgChoice::value_variants()
.iter()
.map(ArgEnum::arg_value)
.map(Option::unwrap)
.collect::<Vec<_>>(),
vec![ArgValue::new("foo"), ArgValue::new("bar")]
);
assert!(ArgChoice::from_str("foo", true).is_ok());
assert!(ArgChoice::from_str("bar", true).is_ok());
assert!(ArgChoice::from_str("baz", true).is_err());
}

#[test]
fn from_str_invalid() {
#[derive(ArgEnum, PartialEq, Debug)]
#[derive(ArgEnum, PartialEq, Debug, Clone)]
enum ArgChoice {
Foo,
}
Expand Down
4 changes: 2 additions & 2 deletions clap_derive/tests/ui/arg_enum_on_struct.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use clap::ArgEnum;

#[derive(ArgEnum, Debug)]
#[derive(ArgEnum, Clone, Debug)]
struct Opt {}

fn main() {
println!("{:?}", Opt::VARIANTS);
println!("{:?}", Opt::value_variants());
}
2 changes: 1 addition & 1 deletion clap_derive/tests/ui/arg_enum_on_struct.stderr
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
error: `#[derive(ArgEnum)]` only supports enums
--> $DIR/arg_enum_on_struct.rs:3:10
|
3 | #[derive(ArgEnum, Debug)]
3 | #[derive(ArgEnum, Clone, Debug)]
| ^^^^^^^
|
= note: this error originates in the derive macro `ArgEnum` (in Nightly builds, run with -Z macro-backtrace for more info)
Loading