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

Const enum variants names #75

Merged
merged 7 commits into from Dec 12, 2019
Merged

Const enum variants names #75

merged 7 commits into from Dec 12, 2019

Conversation

@WaffleLapkin
Copy link
Contributor

@WaffleLapkin WaffleLapkin commented Nov 6, 2019

I tried to write code like this:

pub const LETTERS: &'static [&'static str] = Letter::variants();

#[derive(EnumVariantNames)]
pub enum Letter { A, B, C }

But I've found that variants fn is not const.

This pr changes the behaviour of EnumVariantNames in that way:

  1. Add pub const VARIANTS: &'static [&'static str] = ... to enum
  2. make variants fn const
  3. deprecate variants fn with a message like "Use `Letter::VARIANTS` instead"
WaffleLapkin added 3 commits Nov 6, 2019
@hobofan
Copy link

@hobofan hobofan commented Nov 22, 2019

deprecate variants fn with a message like "Use Letter::VARIANTS instead"

Why deprecate the function if you made the access via the function const too?

@WaffleLapkin
Copy link
Contributor Author

@WaffleLapkin WaffleLapkin commented Nov 22, 2019

Because I see no reasons to use the function, when you can use associated constant.

@Peternator7
Copy link
Owner

@Peternator7 Peternator7 commented Nov 22, 2019

Thanks for the PR! Sorry I thought I responded to this last week, but it looks like I forgot to. Based on #74, I don't think we should deprecate the function because it will be extracted into a trait, and those can't be const yet.

Let's definitely get the associated constant added though :)

@WaffleLapkin
Copy link
Contributor Author

@WaffleLapkin WaffleLapkin commented Nov 22, 2019

Based on #74, I don't think we should deprecate the function because it will be extracted into a trait, and those can't be const yet.

So I must undeprecate variants and make it non-const again? Ok, I'll make these changes tomorrow :)

…`enum_variant_names_inner`
@Peternator7
Copy link
Owner

@Peternator7 Peternator7 commented Nov 24, 2019

Thanks! Merging #74 created a conflict so here's the resolution you'll want in the enum variants file.

    quote! {
        impl #impl_generics ::strum::VariantNames for #name #ty_generics #where_clause {
            /// Return a slice containing the names of the variants of this enum
            #[allow(dead_code)]
            fn variants() -> &'static [&'static str] {
                Self::VARIANTS
            }
        }

        impl #impl_generics #name #ty_generics #where_clause {
            /// Names of the variants of this enum
            #[allow(dead_code)]
            pub const VARIANTS: &'static [&'static str] = &[ #(#names),* ];
        }
    }
@WaffleLapkin
Copy link
Contributor Author

@WaffleLapkin WaffleLapkin commented Nov 25, 2019

@Peternator7 shouldn't I add VARIANTS const to the trait?

@Peternator7
Copy link
Owner

@Peternator7 Peternator7 commented Nov 25, 2019

@WaffleLapkin, that would be preferred, but if I remember correctly, const in traits isn't supported on stable yet :(

@WaffleLapkin
Copy link
Contributor Author

@WaffleLapkin WaffleLapkin commented Nov 27, 2019

@Peternator7 const in traits is supported on stable

@WaffleLapkin
Copy link
Contributor Author

@WaffleLapkin WaffleLapkin commented Nov 30, 2019

@Peternator7 I've added VARIANTS const to VariantNames trait and it seems like there is still no need in variants function... Am I wrong?

@WaffleLapkin
Copy link
Contributor Author

@WaffleLapkin WaffleLapkin commented Dec 11, 2019

@Peternator7
Copy link
Owner

@Peternator7 Peternator7 commented Dec 12, 2019

Thanks for the ping; I agree. Let's go ahead and remove the variants function

@Peternator7 Peternator7 merged commit eea32db into Peternator7:master Dec 12, 2019
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@Peternator7
Copy link
Owner

@Peternator7 Peternator7 commented Dec 12, 2019

Looks great! Thanks for the feature :)

@WaffleLapkin
Copy link
Contributor Author

@WaffleLapkin WaffleLapkin commented Dec 12, 2019

Yay! Btw, don't forget that these changes are breaking now ;)

@WaffleLapkin WaffleLapkin deleted the WaffleLapkin:enum_variant_names_const branch Dec 12, 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