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 building variants to module, derive trait for all variants and marker trait for all variants #7

Merged
merged 9 commits into from Dec 18, 2021

Conversation

kaleidawave
Copy link
Contributor

Found this crate a while back and it's very cool! Had a use case for it today but needed some extra features, so here's my PR for them!

This PR adds 3 features:

Build variants to a module (#5)

As #5 suggests:

#[derive(EnumVariantType)]
#[evt(module = "example")]`
enum Example {
    A,
    B,
}

generates all the variants into a module

mod example {
     ...
}

Which is really useful to prevent naming conflicts and for general structure.

Derive for all variants (#6)

#[derive(EnumVariantType)]
#[evt(derive(Debug, Clone))]`
enum Example {
    A,
    B,
}

Each generated variant has #[derive(Debug, Clone))]

#[derive(Debug, Clone)]
struct A;
#[derive(Debug, Clone)]
struct B;

Useful for generating the variants on large enums with many variants and not having to write out the derive attribute on each variant

Generate and derive marker trait over variants

#[derive(EnumVariantType)]
#[evt(derive_marker_trait(MyExampleMarker))]`
enum Example {
    A,
    B,
}

Generates a marker trait and implements it for each generated variant

trait MyExampleMarker {}
...
impl MyExampleMarker for A {}
impl MyExampleMarker for B {}

This is useful for constraining structures to only use the generated variants, e.g. struct MyExampleHolder<T: MyExampleMarker>(T)


None of the features break existing uses, they only add optional things. I have added tests for each feature. Let me know if there are any changes/alterations/considerations to do...? If/when everything is good I can add the features to the documentation.

Thanks :)

Copy link
Owner

@azriel91 azriel91 left a comment

Choose a reason for hiding this comment

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

heya 👋! Thank you very much for the contribution; am happy to put them in.

The code and tests are very good 🙇, I've added some improvements for the error messages, and one thing that I wasn't sure it's safe to panic on

src/lib.rs Outdated
}
}
} else {
panic!("Invalid evt attr")
Copy link
Owner

Choose a reason for hiding this comment

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

hm, it's been a while since I've done proc macros, but does rust filter out attributes that aren't listed in the proc_macro_derive?

if so, then we can do something like this:

panic!("`EnumVariantType` does not recognize `{}` as an attribute. Did you mean `evt`?")

and if not, we should probably just ignore unrecognized attributes -- I remember something about "proc macros can see all other attributes except derives"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting point, just checked and no rust does not filter attributes not specified in the attribute argument on the proc_macro_derive. For:

#[derive(Debug, EnumVariantType)]
#[evt(...)]
#[rustfmt::ignore]
struct A;

The following attributes are present

ast.attrs = [
    Attribute {
        path: Path {
            leading_colon: None,
            segments: [
                PathSegment {
                    ident: Ident {
                        ident: "evt",
        ...
    },
    Attribute {
        path: Path {
            leading_colon: None,
            segments: [
                PathSegment {
                    ident: Ident {
                        ident: "rustfmt",
        ...
    },
]

And I think this should already be handled as there isn't anything happening on not "evt" attributes. The panic is only if the evt is not a meta list e.g. #[evt] or #[evt = "invalid_use"]

for attr in ast.attrs.iter() {
    if attr.path.is_ident("evt") {
        if let Ok(Meta::List(list)) = attr.parse_meta() {
            ...
        } else {
            panic!("Invalid evt attr")
        }
    }
}

I couldn't think of a better error message 😄 Maybe something which references correct usage or links to it. Also maybe should be returning a compile_error! macro rather than panicking. Will have a look at tidying my error logic up 👍

Copy link
Owner

Choose a reason for hiding this comment

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

And I think this should already be handled as there isn't anything happening on not "evt" attributes. The panic is only if the evt is not a meta list e.g. #[evt] or #[evt = "invalid_use"]

oh yes, my eyes didn't parse that right (though this was at the 2-indent level) 🙇‍♂️

src/lib.rs Outdated
marker_trait_ident = Some(ident);
}
}
_ => panic!("Invalid evt attr"),
Copy link
Owner

Choose a reason for hiding this comment

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

similar to the other comment, can add suggestions of what's valid

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@kaleidawave
Copy link
Contributor Author

Awesome following the conversations have changed the implement_marker_traits thing, improved some error messages, changed some of the tests and have added documentation for the new features. I think I am confident with these changes and they are ready for merging! Feel free if there are any other things todo or you can reword any of the documentation I added.

Wa just re-read your messages and I think you may changed some errors messages but not pushed the changes, feel free to rewrite them over mine.

Copy link
Owner

@azriel91 azriel91 left a comment

Choose a reason for hiding this comment

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

cool, shall merge soon -- I haven't actually written the error message for that outermost case, but shall do that now

src/lib.rs Outdated
}
}
} else {
panic!("Invalid evt attr")
Copy link
Owner

Choose a reason for hiding this comment

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

And I think this should already be handled as there isn't anything happening on not "evt" attributes. The panic is only if the evt is not a meta list e.g. #[evt] or #[evt = "invalid_use"]

oh yes, my eyes didn't parse that right (though this was at the 2-indent level) 🙇‍♂️

@azriel91
Copy link
Owner

ah, it doesn't pick up .github/actions/ci.yml because it lives on the fork. Shall merge and see

@azriel91
Copy link
Owner

cool, got the other random tidy up work done (probably should've done it in its own branch, but ya)

thank you very much for implementing this to a high standard 🙇‍♂️

oh, it's released in 0.3.0 🚀.

@kaleidawave
Copy link
Contributor Author

Awesome, thanks for the quick responses and adding my changes to 0.3.0! 🚀 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants