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

Add EnumVariantNames #56

Merged
merged 6 commits into from Jul 15, 2019
Merged

Conversation

@killercup
Copy link
Contributor

@killercup killercup commented Jun 1, 2019

This derive adds a static variants() methods yielding the names of
the enum variants. This happens to be exactly what clap wants, and
is the last puzzle piece to use strum with clap/structopt.

This derive adds a static `variants()` methods yielding the names of
the enum variants. This happens to be exactly what clap [wants][1], and
is the last puzzle piece to use strum with clap/structopt.

[1]: https://docs.rs/clap/2.33.0/clap/macro.arg_enum.html
@Dushistov
Copy link
Contributor

@Dushistov Dushistov commented Jun 1, 2019

This derive adds a static variants() methods yielding the names of
the enum variants.

Why introduce array while there is EnumIter feature?

@killercup
Copy link
Contributor Author

@killercup killercup commented Jun 1, 2019

This returns a static array of static string slices and is meant as a convenience method.

My main motivation is this: Clap's possible_values methods wants a &[&'b str]. If you only want to use your enum for this purpose, deriving both EnumIter and ToString (and doing something weird to get from the Iterator<Item=String> that &Enum::iter().map(|x| x.to_string()) returns to that type) seems a bit too much boilerplate.

@Dushistov
Copy link
Contributor

@Dushistov Dushistov commented Jun 1, 2019

If you only want to use your enum for this purpose, deriving both EnumIter and ToString (and doing > something weird to get from the Iterator<Item=String> that &Enum::iter().map(|x| x.to_string())
returns to that type) seems a bit too much boilerplate.

My main concern that such functionality already exists and new functionality duplicate it.
There are EnumCount, EnumIter and IntoStaticStr.

May be it is possible to combine them to achive what you want without
introducing one more derive, something like:

#[macro_use]
extern crate strum_macros;
use strum::{EnumCount, IntoEnumIterator};

#[derive(EnumCount, IntoStaticStr, EnumIter)]
enum Boo {
    A,
    B,
    C,
}

fn get_me_static_array<E, I>() -> [&'static str; 3]
where
    E: IntoEnumIterator<Iterator = I> + EnumCount + Into<&'static str>,
    I: Iterator<Item = E>,
{
    let mut arr: [&'static str; 3] = [""; 3/*something like <E as EnumCount>::COUNT*/];
    for (idx, it) in E::iter().enumerate() {
        arr[idx] = it.into();
    }
    arr
}

fn main() {
    println!("{:?}", get_me_static_array::<Boo, _>());
}

This requires only modification of EnumCount trait to introduce associated constant.

@killercup
Copy link
Contributor Author

@killercup killercup commented Jun 1, 2019

Thanks for your comments!

It seems I have misjudged the philosophy of this library, so I'm a bit surprised. You are suggesting to use three of the existing derives (and making one more complex in the process) to not add a new one that is 36 lines of code. I assumed that adding more derives to allow more conveniences was totally in scope. But I guess it is you who will have to maintain this, so it's not my call.

Assuming I agree and we go ahead and introduce get_me_static_array. Where would you put the get_me_static_array function? In a generic trait in strum? How'd you teach people that they need to derive EnumCount, IntoStaticStr, and EnumIter to be able to use that function?

My goal is unchanged; I want to use this with clap/structopt without writing any boilerplate code.

@Dushistov
Copy link
Contributor

@Dushistov Dushistov commented Jun 1, 2019

It seems I have misjudged the philosophy of this library

Take into consideration that I am not maintainer of this crate, I just user and contributor.

And this is point of view of user of strum.
I used/use strum in several crates and I always need different things,
like &[(&'staic str, usize(index))], [&'static str] or [(&'static str, Enum(C like enum))],
and sometimes iterator is enough for me.
So I think it is impractical to add 1 of 10 possible use cases into strum.
But again this is not maintainer point of view.

How'd you teach people that they need to derive EnumCount, IntoStaticStr, and EnumIter to be able to > use that function?

I suppose it can be added as generic "free" function,
and because of traits there is no need to teach, there is list of trait in where bound.
But again this is not maintainer point of view.

@Peternator7
Copy link
Owner

@Peternator7 Peternator7 commented Jun 4, 2019

I appreciate the discussion and the PR!

I agree it's a little redundant, but my inclination is to merge it anyway. Here's my thinking:

  1. This project by it's very nature is about removing boilerplate code even if it is pretty simple to write manually. I know it's impractical to cover all combinations of things that people might want, but supporting a scenario that allows better integration with other popular packages makes sense to me.
  2. I've wished strum and clap/structopt had better integration for a while now, but never had time to look into it. This seems like a natural first step towards tighter integration.
  3. Philosophically, I try to include as many PR's as reasonably possible. As long as the docs are good, maintenance cost is low, and it makes it easier for people to find them if they're all in 1 package together.

@killercup I'll give the code a review in the next day or 2 :)

@killercup
Copy link
Contributor Author

@killercup killercup commented Jun 24, 2019

Ping on review? Not urgent, just one of the dependencies I use a local fork for right now :)

strum_macros/src/enum_variant_names.rs Outdated Show resolved Hide resolved
strum_macros/src/enum_variant_names.rs Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@Peternator7
Copy link
Owner

@Peternator7 Peternator7 commented Jun 24, 2019

Whoops I forgot to submit the review. thanks for the ping

killercup added 4 commits Jun 27, 2019
This reduces the risk of breaking the public API by accident when adding
a new variant.
@killercup
Copy link
Contributor Author

@killercup killercup commented Jun 27, 2019

Thanks! That's super good feedback! I've pushed some updates that address all your comments and add tests asserting that this actually works with clap and structopt.

@Peternator7 Peternator7 merged commit 96daaf4 into Peternator7:master Jul 15, 2019
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@mandrean mandrean mentioned this pull request Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants