deer implement value deserializers
#1947
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1947 +/- ##
==========================================
+ Coverage 56.12% 57.94% +1.81%
==========================================
Files 269 266 -3
Lines 19459 18756 -703
Branches 421 421
==========================================
- Hits 10922 10868 -54
+ Misses 8532 7883 -649
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
indietyp
changed the title
deer implement value deserializers
| pub struct BorrowedBytesDeserializer<'a, 'de> { | ||
| context: &'a Context, | ||
| value: &'de [u8], | ||
| } |
There was a problem hiding this comment.
What are the differences for BorrowedBytesDeserializer and others (StrDeserializer, NoneDeserializer, etc.) that can't use impl_owned? Is is the fact that they require additional lifetimes to be specified?
There was a problem hiding this comment.
That is correct, the additional lifetimes would complicate the macro too much, so for now, I did not include them.
| macro_rules! forward_to_deserialize_any_method { | ||
| ($func:ident < $l:tt, $v:ident > ()) => { | ||
| #[inline] | ||
| fn $func<$v>(self, visitor: $v) -> error_stack::Result<$v::Value, $crate::DeserializerError> | ||
| where | ||
| $v: $crate::Visitor<$l>, | ||
| { | ||
| self.deserialize_any(visitor) | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
Would it not be sufficient to provide default impls. for all these in Deserializer instead of these macros or are we reyling on being able to specify generics in a complicated manner?
There was a problem hiding this comment.
I know the pain; this is possible, but it would lead implementors to accidentally miss a function when implementing. This way, they need to implement every function without accidentally missing a deserializer function, and if they want to, they can use the macro to short-circuit. This is also what serde does.
So while technically possible, it would make potential new implementations more error-prone, IMO.
What do you think?
There was a problem hiding this comment.
I expected that to be the reason. A subtle bug due to missing a case seems way more problematic than having some macro indirection.
I do think that an extensive test suite should pick up any problems with that and make the default impls a better option. What about we keep this for now, but try to use default implementations at some point in the future when we're confident (i.e. we have extensive tests)?
There was a problem hiding this comment.
I am not 100% confident with either (default implementation would be ok for us, but foreign crates implementing Deserializer would suffer if they do not have extensive tests). As you outlined above, the current version also has problems associated with it.
I'd move this to the backlog for now, and maybe through discussion, we are able to come to a better solution! :D I am very much open to ideas.
But I do not expect that there's a large number of implementations for Deserializer (only data formats will implement them), see serde or any other serialization/deserialization crate, there are a dozen Deserializer implementations but thousands of Deserialize implementations. So the tradeoff macro vs. default impl is hard to quantify 🤔 Maybe once we touch documentation an implementation guide could help with that.
There was a problem hiding this comment.
Sure, sounds good. I am okay with merging this right now, then :)
|
/trunk merge |
|
😎 Merged successfully (details). |
🌟 What is the purpose of this PR?
These deserializers only act upon a single value and are inspired by
serde. They enable us to easily implement types likeOption<T>.🔗 Related links
📹 Demo