Refactoring logic to handle enum newtype variants#949
Refactoring logic to handle enum newtype variants#949coderBlitz wants to merge 1 commit intotafia:masterfrom
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #949 +/- ##
==========================================
+ Coverage 55.00% 56.42% +1.42%
==========================================
Files 44 44
Lines 16816 17599 +783
==========================================
+ Hits 9249 9931 +682
- Misses 7567 7668 +101
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Mingun
left a comment
There was a problem hiding this comment.
Before submitting PR I want to hear how the following types should be mapped to the XML, and why (if their mapping will differ from current), for all possible enum variants (unit, newtype, tuple, struct):
EnumRoot { field: Enum }Root { #[serde(flatten)] field: Enum }Root { #[serde(rename = "$text")] field: Enum }Root { #[serde(rename = "$value")] field: Enum }
Mapping must be consistent and not create WTF situations.
Mapping must be composable, that means if you know how the type will be serialized, you must predict how it will be serialized (or cannot be serialized) when you put it in:
- a map key/value
- a value of a struct field
- a value of a newtype
- a value of a tuple element
- a
Somevalue of anOption
Serialized XML must be deserializable to the original type. If exceptions exists, they should be explained.
| // #XXX - Removed since it seemingly only fails when | ||
| // deserializing tuple variant. | ||
| //debug_assert_eq!(self.start.name(), e.name()); |
There was a problem hiding this comment.
If it failed here, your code contains logical error.
| seed.deserialize(BorrowedStrDeserializer::<DeError>::new(TEXT_KEY))?, | ||
| true, | ||
| ), | ||
| // SAFETY: we use that deserializer only when we peeked `Start` or `Text` event |
There was a problem hiding this comment.
SAFETY markers shouldn't be removed. They should be updated with the explanation, why that code is unreachable.
| } | ||
|
|
||
| match self.map.de.next()? { | ||
| // Handles <field>UnitEnumVariant</field> |
There was a problem hiding this comment.
That commend should be updated to show what XML shape processed here. This is required because the code became much more complicated
| let mut has_text_key = false; | ||
| for variant in variants { | ||
| if *variant == "$text" { | ||
| has_text_key = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
This code conveys intent more clearly:
| let mut has_text_key = false; | |
| for variant in variants { | |
| if *variant == "$text" { | |
| has_text_key = true; | |
| } | |
| } | |
| let has_text_key = variants.contains(TEXT_KEY); |
| let matches_variant = false; | ||
| for variant in variants { | ||
| // If matching variant found, then decode variant. | ||
| if t.buf == variant.as_bytes() { |
There was a problem hiding this comment.
That is wrong check. buf is in t.decoder() encoding, while variant is in UTF-8.
I need this crate to support enum newtype variants if possible, and these changes so far seem to do just that. Though I only intended to solve the newtype variant issue, some quick testing seems to show that the deserialize changes actually work for all enum variants now (see below)! This would addresses a significant portion of #717, leaving only the serialization of the tuple and struct variants to be done; also the documentation portion.
I did modify some of the test cases, but everything still passes.
Quick examples: