Peternator7 / strum

A small rust library for adding custom derives to enums
https://crates.io/crates/strum
MIT License
1.8k stars 152 forks source link

Add Enum Map Derive Macro #279

Closed PokeJofeJr4th closed 10 months ago

PokeJofeJr4th commented 1 year ago

This started as Issue #272

#[derive(EnumMap)]
enum MyEnum {
  VariantName,*
}

This macro creates a new type MyEnumMap<T> with a field of type T for each variant of MyEnum. It also adds the following:

Questions:

PokeJofeJr4th commented 1 year ago

the build is only failing because the version of rust they're using is out of date

itsjunetime commented 1 year ago

In response to some of your other questions which I've been thinking about:

  1. I'm not quite certain what a better name would be. I guess if there was some way to specify that it's a map that has Some value for every possible key, that could be more clear, but I don't know what that would be.
  2. I feel like the all_ok function is definitely useful, but would be better if it returned a Result<T, E> where it returns Err with the first error encountered while looking through the variants (plus, this would also make it really simple to implement - just EnumMap { #(variant: variant?,)* } in the body). I was previously going to say that all wouldn't be super useful, but I was thinking about my use cases and realized it could actually be really useful. E.g. I would like to store data in an EnumMap to disk, and the library I'm using for that can store a HashMap right out of the box. So I plan to convert the EnumMap to a HashMap when storing like so:

    let mut map = HashMap::new();
    for var in Enum::iter() {
        map[var] = enum_map[var]
    }

    But to convert back after reading the HashMap from the disk, The only safe and clean way I can think of (besides manually getting the value for each variant) would be:

    let Some(enum_map) = EnumMap::from_closure(|v| hash_map.get(v)).all() else {
        println!("oops!");
    };

    So actually, I feel like all could definitely be useful if you need to convert from other similar storage mediums.

  3. I put my thoughts about non-unit variants in another comment
  4. I think impling Clone where T: Clone could be nice. I guess if you wanted to work around it, you could just do EnumMap::from_closure(|v| other_map[v].clone()), but just doing other_map.clone() would be cleaner and clearer a bit.
PokeJofeJr4th commented 1 year ago
  1. ChatGPT's best suggestion was MyEnumTable, which I kinda like
  2. I did change the all_ok to what you suggested. However, for your exact case, would it be better to implement a way to directly store the EnumMap?
  3. yeah that's still a hard one
  4. That's already a thing with the #[derive(...)] thingy; I've added a test to clarify that
itsjunetime commented 1 year ago
  1. Yeah, I think calling it a table could be good; if people would prefer that, then I have no objections
  2. It would definitely be better for me to directly store the EnumMap, but with the library that I'm using, that requires deriveing a bespoke macro, which I don't think I can do right now for the generated EnumMap. However, with that in mind, perhaps we could add an attribute like #[enum_map(derive = "MyTrait, MyOtherTrait")] so that you can add other traits to the generated struct's derive attribute? I'm not attached to that syntax, just the general idea of adding extra traits to derive.

And thanks for pointing out that it already derives Clone; I must've missed that.

PokeJofeJr4th commented 1 year ago

found a significant issue https://github.com/PokeJofeJr4th/strum/issues/8

Peternator7 commented 1 year ago

Could you clarify what the significant issue was? It's good to consider that someone could use a reserved word; can we fix that by prepending an underscore to their identifier?

PokeJofeJr4th commented 1 year ago

Sorry, I see now that you have a perfectly fine fix (prepending _). I previously thought we'd have to figure out if it was a reserved keyword and use r# to escape it; but since it's not publicly facing the underscore solution is perfectly valid

itsjunetime commented 1 year ago

I feel like it would be really useful to add custom derive commands for each struct this is used on, e.g.

#[derive(EnumTable)]
#[strum(table_derives = "Serialize, Deserialize")]
enum Color {
...
}

this way, ColorTable would have #[derive(Serialize, Deserialize)] added on its declaration since you can't do that otherwise (at least, as far as I know). Would anyone be opposed to this?

PokeJofeJr4th commented 1 year ago

I like that but will the namespaces work on it? Would the user having macros like Serialize and Deserialize in the same scope as #[derive(EnumTable)] be enough?

Peternator7 commented 10 months ago

Published strum version 0.26.0 that includes this feature. I've labeled it experimental because I'm not totally sure the api surface won't change, but think it's an interesting start. Thanks for the work on this and sorry for the delay merging it.