Add serde support for Value #16
Conversation
XAMPPRocky
force-pushed
the
add-value
branch
from
9521c97 to
2ad717e
Compare
| } | ||
| } | ||
|
|
||
| struct KindVisitor; |
There was a problem hiding this comment.
I think you might be able to avoid defining this visitor manually by using untagged. This is how numbers are deserialized from either strings or numbers in pbjson. This way is fine also though
FYI there is an open serde issue to expose the plumbing that serde-derive is using - serde-rs/serde#1947
| #[cfg(test)] | ||
| mod tests { | ||
| #[test] | ||
| fn float_special_cases() { |
| ser, Deserialize, Deserializer, Serialize, Serializer, | ||
| }; | ||
|
|
||
| macro_rules! from { |
| @@ -0,0 +1,317 @@ | |||
| pub use crate::pb::google::protobuf::value::Kind; | |||
There was a problem hiding this comment.
Unless I'm mistaken this module is crate-private and so the pub use is really pub(crate) use. Is this intentional?
I guess I'm just wondering why this isn't just use crate::value::Kind?
There was a problem hiding this comment.
Was not intentional for this to be private, this is intended to replace the value module that is in google::protobuf.
There was a problem hiding this comment.
See comment above, I think I would prefer to keep the module topology flat unless there is a compelling reason to not do so - e.g. new public types
XAMPPRocky
force-pushed
the
add-value
branch
from
2ad717e to
6497ac2
Compare
|
@tustvold This should be ready for a re-review. |
| mod timestamp; | ||
| pub mod value; |
There was a problem hiding this comment.
I think I would prefer to keep the module topology flat, currently all inner modules are private and the types are re-exported by the pub use pb::google::protobuf::* a few lines down
value does not appear to define any new public types, but just define some trait impls for crate::Value. Is there a particular reason you wish to have a value module?
There was a problem hiding this comment.
Is there a particular reason you wish to have a value module?
If I was looking for the implementation of Value, intuitively I'd look for src/value.rs, the topology isn't flat because pb::google::protobuf::* already exports a value module. So this makes so the actual code structure reflect what people see in the documentation, and removes the conflicts that arise from using glob exports when it is private. mod value overrides the value from pb::google::protobuf::*, but this wouldn't be obvious to you unless you removed the pub mod and pub use pb::protobuf::google::protobuf::Kind and then tried to use crate::value::Kind.
There was a problem hiding this comment.
Oh, yes prost is generating a value namespace within pb::google::protobuf because Kind is defined within the Value message. Gotcha, that is definitely confusing but makes sense 👍
| use crate::Value; | ||
|
|
||
| #[test] | ||
| fn list_value() { |
There was a problem hiding this comment.
Could we get a test with some mixed types, e.g. something like
assert_eq!(
serde_json::to_value(Value::from([true.into(), "HELLO".into(), false.into()])).unwrap(),
serde_json::json!([serde_json::json!(true), serde_json::json!("HELLO"), serde_json::json!(false)])
);
Or even some nested maps if you're feeling fancy
There was a problem hiding this comment.
Added the mixed types test, nested maps is already tested in the struct::it_works test. Since ListValue is just using Value's serde impl for items, there isn't a chance that ListValue would produce a different value for nested maps than Struct.
| @@ -0,0 +1,317 @@ | |||
| pub use crate::pb::google::protobuf::value::Kind; | |||
There was a problem hiding this comment.
See comment above, I think I would prefer to keep the module topology flat unless there is a compelling reason to not do so - e.g. new public types
tustvold
added
automerge
|
🎉 Thanks again 😄 |
This PR adds
serdesupport forgoogle.protobuf.Value.closes #5
Depends on #14