Derive value improvements for enums #540
Conversation
katyo
commented
Jan 6, 2024
katyo
commented
- Support enums without explicit values
- Support string representation for enums
|
As a future improvement: what about using external crate for ident case conversion? |
There was a problem hiding this comment.
Thanks for this contribution! Some nits to fix. Talking of which, could you kindly add emoji prefixes as per our contribution guidelines?
I'm not sure what you mean but I'd like to avoid adding a dep unless there is a compelling rationale presented. :) |
|
@katyo Hi there. Do you think you'll have time to finish this soon? No hurry, just a polite ping. :) |
|
@zeenix I'm a little bit busy lately, but I try fix this pr asap. |
katyo
force-pushed
the
derive-value-enum
branch
3 times, most recently
from
f60a387 to
57bb1ce
Compare
katyo
force-pushed
the
derive-value-enum
branch
from
57bb1ce to
69ca851
Compare
| @@ -1496,8 +1496,12 @@ mod tests { | |||
| let decoded: Enum = encoded.deserialize().unwrap().0; | |||
| assert_eq!(decoded, Enum::Variant3); | |||
|
|
|||
| assert_eq!(Value::from(Enum::Variant1), Value::U8(0)); | |||
| assert_eq!(Enum::try_from(Value::U8(2)), Ok(Enum::Variant3)); | |||
| assert_eq!(Enum::try_from(Value::U8(4)), Err(Error::IncorrectType)); | |||
There was a problem hiding this comment.
Nice! I think you want to also update the docs.
| #[derive(Deserialize, Serialize, Type, Debug, PartialEq)] | ||
| #[zvariant(signature = "s")] | ||
| #[derive(Deserialize, Serialize, Type, Value, OwnedValue, Debug, PartialEq)] | ||
| #[zvariant(signature = "s", rename_all = "snake_case")] |
There was a problem hiding this comment.
Would be nice to document these additions in the macro docs.
| } | ||
| } | ||
|
|
||
| #into_value | ||
| }) | ||
| } | ||
|
|
||
| fn enum_name_for_variant( |
There was a problem hiding this comment.
SerializeDict and DeserializeDict already support these renaming attributes so I'm sure there is existing code you can use.
There was a problem hiding this comment.
Of course, but currently dict_name_for_field expect syn::Field as an argument, not something like Literal or String. I would like to avoid modifying existing code which is not related to this PR.
There was a problem hiding this comment.
It is related to this PR since there is no good reason to duplicate the code. AFAIK, it's a pretty trivial change so why not just do it? It will need to be renamed and moved to utils mod btw.
katyo
force-pushed
the
derive-value-enum
branch
from
69ca851 to
0f102fc
Compare
Just a gentle reminder, in hope that you're not super busy and can finish this PR. 🙏 |
I guess not. 😞 |
