Add default_with strum macro #254
Conversation
ericmcbride
commented
Feb 26, 2023
•
ericmcbride
commented
- My first shot at messing with Syn + Quote + Proc Macros in general
- Added the default_with macro, from Feature Request: Add attribute for types that don't implement Default #245
- Added tests to cover new macro
- Will Add doc strings after verifying if my approach is ok
| let defaults = | ||
| ::core::iter::repeat(quote!(Default::default())).take(fields.unnamed.len()); | ||
| quote! { (#(#defaults),*) } | ||
| if let Some(ref value) = variant_properties.default_with { |
There was a problem hiding this comment.
I wasn't sure if there was a new-type variant, if we wanted to be able to default each unnamed field. It'll be easy enough to add this feature/tests
There was a problem hiding this comment.
If it's not too difficult, let's try to match the conventions that serde uses according to this doc:
https://serde.rs/variant-attrs.html#deserialize_with
#[serde(deserialize_with = "path")]Deserialize this variant using a function that is different from its implementation of Deserialize. The given function must be callable as
fn<'de, D>(D) -> Result<FIELDS, D::Error> where D: Deserializer<'de>, although it may also be generic over the elements of FIELDS. Variants used with deserialize_with are not required be able to derive Deserialize.FIELDS is a tuple of all fields of the variant. A unit variant will have () as its FIELDS type.
I'm not too worried about the generic bounds parts, however, if we can return a tuple, that's probably the clearest way to do it. Thoughts?
There was a problem hiding this comment.
Let me see what I can do!
| let mut defaults = vec![]; | ||
| for field in &fields.named { | ||
| let meta = field.get_variant_inner_properties()?; | ||
| let field = field.ident.as_ref().unwrap(); |
There was a problem hiding this comment.
Wasn't sure about the unwrap here. I don't like unwraps in general, and I could return an ok_or_else Syn error here. I thought it'd just be weird if an ident on a field wasn't a thing. Thoughts here?
There was a problem hiding this comment.
I don't really mind unwrapping/expecting here. Maybe just use expect and tell people to file an issue against strum if they see this message
| @@ -15,6 +15,15 @@ enum Color { | |||
| Purple, | |||
| #[strum(serialize = "blk", serialize = "Black", ascii_case_insensitive)] | |||
| Black, | |||
| Pink { | |||
| #[strum(default_with = "test_default")] | |||
There was a problem hiding this comment.
Let's add a test to make sure it works properly with a full module path
| let defaults = | ||
| ::core::iter::repeat(quote!(Default::default())).take(fields.unnamed.len()); | ||
| quote! { (#(#defaults),*) } | ||
| if let Some(ref value) = variant_properties.default_with { |
There was a problem hiding this comment.
If it's not too difficult, let's try to match the conventions that serde uses according to this doc:
https://serde.rs/variant-attrs.html#deserialize_with
#[serde(deserialize_with = "path")]Deserialize this variant using a function that is different from its implementation of Deserialize. The given function must be callable as
fn<'de, D>(D) -> Result<FIELDS, D::Error> where D: Deserializer<'de>, although it may also be generic over the elements of FIELDS. Variants used with deserialize_with are not required be able to derive Deserialize.FIELDS is a tuple of all fields of the variant. A unit variant will have () as its FIELDS type.
I'm not too worried about the generic bounds parts, however, if we can return a tuple, that's probably the clearest way to do it. Thoughts?
| let mut defaults = vec![]; | ||
| for field in &fields.named { | ||
| let meta = field.get_variant_inner_properties()?; | ||
| let field = field.ident.as_ref().unwrap(); |
There was a problem hiding this comment.
I don't really mind unwrapping/expecting here. Maybe just use expect and tell people to file an issue against strum if they see this message
|
Sorry for the long delay on this :( I don't have much time for this project anymore, but I'm trying to get through everything in the backlog. I think this is a cool addition to the project |
