-
Notifications
You must be signed in to change notification settings - Fork 20
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
Implement valuable-serde #23
Conversation
valuable-serde/tests/test.rs
Outdated
// TODO | ||
// Unit, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, Valuable
treats the unit variant/struct as an empty tuple variant/struct. Therefore, I'm not sure if we can provide a behavior that is fully consistent with serde on unit variant/struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems related to #21, maybe provide some thoughts there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem I encountered here is mainly related to the unit struct, not to tuples. But in any case, we need to think about how to represent tuples...
valuable/valuable-serde/tests/test.rs
Lines 85 to 101 in 9a68fc3
// TODO: | |
// - valuable treats unit struct as an empty tuple struct | |
// - serde treats unit struct as unit struct | |
#[test] | |
fn test_unit_struct() { | |
#[derive(Debug, PartialEq, Valuable, Serialize)] | |
struct S; | |
assert_ser_tokens( | |
&Serializable::new(S), | |
&[ | |
Token::TupleStruct { name: "S", len: 0 }, | |
Token::TupleStructEnd, | |
], | |
); | |
assert_ser_tokens(&S, &[Token::UnitStruct { name: "S" }]); | |
} |
// TODO: | ||
// - valuable treats Option<T> as T or unit | ||
// - serde treats Option<T> as Some(T) or None | ||
#[test] | ||
fn test_option() { | ||
assert_ser_tokens(&Serializable::new(None::<u8>), &[Token::Unit]); | ||
assert_ser_tokens(&None::<u8>, &[Token::None]); | ||
assert_ser_tokens(&Serializable::new(Some(1)), &[Token::I32(1)]); | ||
assert_ser_tokens(&Some(1), &[Token::Some, Token::I32(1)]); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- valuable treats Option<T> as T or unit
- serde treats Option<T> as Some(T) or None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we can consider breaking out Some
and None
in Value
?
I think this is now ready for review. Currently, I'm aware of two differences in behavior from serde. See #23 (comment) and #23 (comment) for more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Could you file follow up issues w/ the remaining unknowns?
filed #42 to track remaining todos. |
Closes #17
Examples