Add serde Ext support for rmpv and rmps#216
Conversation
daboross
left a comment
daboross
left a comment
There was a problem hiding this comment.
Thanks for writing this!
I've included a number of comments on things I think could be cleaned up a bit. Nothing major, mostly just error messages and removing some of the code in favor of using pre-built alternatives.
Besides the existing code, do you think it'd be possible to get this working with serde_derive-created Serialize and Deserialize implementations?
It looks like it's almost possible to store ext data in something like:
#[derive(Serialize, Deserialize, PartialEq, Eq, Debug)]
#[serde(rename = "_ExtStruct")]
struct ExtStruct(i8, Vec<u8>);I've tested it locally, and round-trip tests don't quite succeed, but I think this implementation is fairly close. If we got this working, it seems like it could make for a nicer interface. Thoughts on that?
Regardless, this looks like a good addition to me. I'm good with approving this with or without serde_derive support after fixing the other comments. Thanks!
|
Thank you for your very detailed review. I will fix it all and try to look at the serde_derive support. |
* New Ext Data Model: _ExtStruct((tag, binary)) * Fix Review * Add Serde Derive test
|
I have to change Serde Data Model of Ext to a newtype struct named |
daboross
left a comment
daboross
left a comment
There was a problem hiding this comment.
Thanks for working on this! Sorry I wasn't able to make a review earlier.
This is because, for
ValueandValueRef, which rely ondeserialize_anyto visit the correct type, there's currently no way to communicate the seq's name from deserializer to visitor.
Makes sense - I'm slightly sad about making it more complex, but I think it's worth it.
Looking over this, I think all the code looks good. A few bits I might have written differently, but that isn't a bad thing.
If we fix the two code blocks in the doc-comments, I'd be happy with this. Thanks again!
|
@kornelski This looks good to me. If you're good with the addition, I'm good with merging it. |
Tags are stored as Newtype Structs with the special name `_TagStruct`. The tags
are represented as a struct tuple consisting of the tag identifier and a value.
Example:
let tag = Value::Tag(123, Box::new(Value::Text("some value".to_string())));
let tag_encoded = to_vec(&tag).unwrap();
This implementation is heavily based on the msgpack-rust library, when support
for extensions were added [1][2]. They have the same problem as CBOR has, that
such an extension/tagging type is not directly supported by Serde, hence a
workaround is needed. For me it makes sense to use a similarly working workaround
across different libraries.
This commit also contains an example on how to use custom types as tags. It can
be run via:
cargo run --example tags --features tags
[1]: 3Hren/msgpack-rust@a34ab8f
[2]: 3Hren/msgpack-rust#216
Tags are stored as Newtype Structs with the special name `_TagStruct`. The tags
are represented as a struct tuple consisting of the tag identifier and a value.
Example:
let tag = Value::Tag(123, Box::new(Value::Text("some value".to_string())));
let tag_encoded = to_vec(&tag).unwrap();
This implementation is heavily based on the msgpack-rust library, when support
for extensions were added [1][2]. They have the same problem as CBOR has, that
such an extension/tagging type is not directly supported by Serde, hence a
workaround is needed. For me it makes sense to use a similarly working workaround
across different libraries.
This commit also contains an example on how to use custom types as tags. It can
be run via:
cargo run --example tags --features tags
[1]: 3Hren/msgpack-rust@a34ab8f
[2]: 3Hren/msgpack-rust#216
Tags are stored as Newtype Structs with the special name `_TagStruct`. The tags
are represented as a struct tuple consisting of the tag identifier and a value.
Example:
let tag = Value::Tag(123, Box::new(Value::Text("some value".to_string())));
let tag_encoded = to_vec(&tag).unwrap();
This implementation is heavily based on the msgpack-rust library, when support
for extensions were added [1][2]. They have the same problem as CBOR has, that
such an extension/tagging type is not directly supported by Serde, hence a
workaround is needed. For me it makes sense to use a similarly working workaround
across different libraries.
This commit also contains an example on how to use custom types as tags. It can
be run via:
cargo run --example tags --features tags
[1]: 3Hren/msgpack-rust@a34ab8f
[2]: 3Hren/msgpack-rust#216
Tags are stored as Newtype Structs with the special name `_TagStruct`. The tags
are represented as a struct tuple consisting of the tag identifier and a value.
Example:
let tag = Value::Tag(123, Box::new(Value::Text("some value".to_string())));
let tag_encoded = to_vec(&tag).unwrap();
This implementation is heavily based on the msgpack-rust library, when support
for extensions were added [1][2]. They have the same problem as CBOR has, that
such an extension/tagging type is not directly supported by Serde, hence a
workaround is needed. For me it makes sense to use a similarly working workaround
across different libraries.
This commit also contains an example on how to use custom types as tags. It can
be run via:
cargo run --example tags --features tags
[1]: 3Hren/msgpack-rust@a34ab8f
[2]: 3Hren/msgpack-rust#216
3 participants
No description provided.