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

Re-export macros #170

Merged
merged 6 commits into from
Jul 17, 2021
Merged

Conversation

billy1624
Copy link
Contributor

Motivation

Our team is developing a library which requires downstream end-user to derive strum_macro::EnumIter for their custom enum.

pub trait RelationTrait: Iterable + Debug + 'static { ... }

// Derive EnumIter
#[derive(Copy, Clone, Debug, EnumIter)]
pub enum Relation {
    Cake,
    Filling,
}

However, the expanded codes from strum_macro::EnumIter looks something like this and failed to compile...
This signature, ::strum::IntoEnumIterator, implies our downstream end-user must add the strum dependency on their own.

impl ::strum::IntoEnumIterator for Relation { ... }
error[E0433]: failed to resolve: could not find `strum` in the list of imported crates
   |
39 | #[derive(Copy, Clone, Debug, EnumIter)]
   |                              ^^^^^^^^ could not find `strum` in the list of imported crates
   |

Hence, we want to find a way to re-export strum & strum_macros in our library. And downstream end-user don't need to specify the strum dependency on their own.

Proposed Solution

Adding an optional derive attribute #[strum(crate_path = "some_crate::strum")], for example...

// Derive EnumIter with crate_path derive attribute
#[derive(Copy, Clone, Debug, EnumIter)]
#[strum(crate_path = "some_crate::strum")]
pub enum Relation {
    Cake,
    Filling,
}

// Derive macro will be expanded into
impl some_crate::strum::IntoEnumIterator for Relation { ... }

Backward compatibility, if the derive attribute was not provided...

// Derive EnumIter without crate_path derive attribute
#[derive(Copy, Clone, Debug, EnumIter)]
pub enum Relation {
    Cake,
    Filling,
}

// Derive macro will be expanded into
impl ::strum::IntoEnumIterator for Relation { ... }

References

Similar feature was discussed on serde-rs/serde#1465. And serde provides a optional derive attribute to achieve it.

#[derive(Copy, Clone, Debug, Deserialize, Serialize)]
#[serde(crate = "some_crate::serde")]
struct Vertex {
    position: [u32; 3],
}

Copy link
Owner

@Peternator7 Peternator7 left a comment

Choose a reason for hiding this comment

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

This makes a lot of sense to me! Thanks for putting together a PR :) I left a few comments and if you could add a few tests, that would be great.

Maybe just try re-exporting the strum derives from some nested module in the tests and make sure it still works. You could also use the debugging section to make sure the proper code is generated.

@@ -14,6 +15,7 @@ pub mod kw {

// enum metadata
custom_keyword!(serialize_all);
custom_keyword!(crate_path);
Copy link
Owner

Choose a reason for hiding this comment

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

Did you say serde uses "crate?" Can we also do that for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

crate is a Rust keyword, is it okay if I use Crate instead?

Copy link
Owner

Choose a reason for hiding this comment

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

sorry for the delay getting back to you. thanks for your patience. I would prefer crate if we can since that's what serde does. Is there a technical reason we can't do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just overcome the technical difficulties, see the latest commit, now we have

#[derive(Debug, Eq, PartialEq, EnumIter)]
#[strum(crate = "nested::module::strum")]
enum Week {
    Sunday,
    Monday,
    Tuesday,
    Wednesday,
    Thursday,
    Friday,
    Saturday,
}

strum_macros/src/macros/enum_count.rs Outdated Show resolved Hide resolved
strum_macros/src/macros/enum_iter.rs Outdated Show resolved Hide resolved
@billy1624
Copy link
Contributor Author

Please review @Peternator7

@Peternator7
Copy link
Owner

lgtm! Thanks for the contribution. I'm on vacation next week, but I'll try and get a release out if I find a little time!

@Peternator7 Peternator7 merged commit e9d3b59 into Peternator7:master Jul 17, 2021
@billy1624
Copy link
Contributor Author

Have a nice vacation!!

@KeyboardDanni
Copy link

Hi, this would be very useful for my crate, as I'm in the process of refactoring my framework to improve ergonomics, and strum is currently used as an implementation detail. It would be nice to be able to put this into a macro and not require the user to manually pull in the strum crates.

Could a new release be made on crates.io so I can use this? Thanks!

@Peternator7
Copy link
Owner

Hi @cosmicchipsocket and @billy1624, thanks for the push. Version 0.22 is live on crates.io

@billy1624
Copy link
Contributor Author

Thanks! @cosmicchipsocket

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.

3 participants