-
Notifications
You must be signed in to change notification settings - Fork 267
Fix serde 1.0.181 compatibility #669
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
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…e_*_variant`s
The code was simply copied with adaptation of parameter names.
This is required, because we want to change representation of internal `enum` fields
to match the current `$text` representation, but keep representation of top-level
enums unchanged (because there are no field for them that could be renamed to `$value`)
That means that
enum Enum {
Unit,
}
would be represented differently when used as a top-level type or as a type of field:
<!-- top-level representation -->
<Unit/>
<!-- in-field representation (in field with name "field") -->
<field>Unit</field>
We need to change default in-field representation to be compatible with serde changes
since >=1.0.181. To get the old representation we need to rename the field to `$value`.
Actually, this is even more correct way, because I noticed, that currently serializer
and deserializer worked differently and a `de` module documentation contains an error,
which was unnoticed, because that code snippet is not tested. That will be fixed in next
commits.
This commit simply moves pieces of code from one place to another
After inlining we could notice that some unnecessary work are done. This commit slightly changes the behavior, because construction of XmlName checks the allowed names to be correspond to XML 1.1 rules for names. This is not a problem, because we do not want to check names that will not be used in the generated XML.
That way actual types will be shown in the documentation.
The new names more precisely reflects their purpose
…o ContentSerializer tests In the next commits representation of enums will change and ElementSerializer won't anymore support serialization of enum struct variants. In order to not lost tests move them into ContentSerializer. It is also not meaningless (for `$value`), since it is ContentSerializer which is responsible for serializing `$value` fields and struct variants will be possible to serialize only in `$value` fields, i. e. they will be serialized by ContentSerializer. For the `$text` fields tests is moved only to keep them.
Actually, our documentation was incorrect because test is failing.
All other commits in this branch will have a goal to fix that.
failures:
src\de\mod.rs - de (line 1434)
…erent kind of fields
failures (10):
without_root::enum_::externally_tagged::text_variant::normal_field::newtype
without_root::enum_::externally_tagged::text_variant::normal_field::tuple
without_root::enum_::externally_tagged::text_variant::normal_field::tuple_with_spaces
without_root::enum_::externally_tagged::text_variant::normal_field::unit
without_root::enum_::externally_tagged::text_variant::text_field::unit
without_root::enum_::externally_tagged::text_variant::value_field::newtype
without_root::enum_::externally_tagged::text_variant::value_field::struct_
without_root::enum_::externally_tagged::text_variant::value_field::tuple
without_root::enum_::externally_tagged::text_variant::value_field::tuple_with_spaces
without_root::enum_::externally_tagged::text_variant::value_field::unit
…ontentSerializer
ContentSerializer is used to write inner values of tags;
the `$value` name disables writing surrounding tags from fields.
(review with "ignore whitespace changes" option on)
failures (5):
without_root::enum_::externally_tagged::text_variant::normal_field::newtype
without_root::enum_::externally_tagged::text_variant::normal_field::tuple
without_root::enum_::externally_tagged::text_variant::normal_field::tuple_with_spaces
without_root::enum_::externally_tagged::text_variant::normal_field::unit
without_root::enum_::externally_tagged::text_variant::text_field::unit
Fixed (5):
without_root::enum_::externally_tagged::text_variant::value_field::newtype
without_root::enum_::externally_tagged::text_variant::value_field::struct_
without_root::enum_::externally_tagged::text_variant::value_field::tuple
without_root::enum_::externally_tagged::text_variant::value_field::tuple_with_spaces
without_root::enum_::externally_tagged::text_variant::value_field::unit
New representation policy is compatible with serde>=1.0.181 and also more correct
and consistent with general mapping rules.
General rules of new policy:
- In `$value` field the representation is always the same as top-level representation;
- In `$text` field the representation is always the same as in normal field, but surrounding tags with field name are removed;
- In normal field the representation is always contains a tag with field name.
The tables below summarizes the representation policy. For examples in fields only content
at place of `...` is shown:
```
<Container>...</Container>
```
The struct which contains field looks like:
```
struct Container {
field: Enum,
#[serde(rename = "$text")]
text: Enum,
#[serde(rename = "$value")]
value: Enum,
}
```
Normal enum variant
-------------------
### Old (before this commit) representation
|Kind |Top-level, in normal and `$value` fields |In `$text` field|
|-------|-----------------------------------------|----------------|
|Unit |`<Unit/>` |_(empty)_ |
|Newtype|`<Newtype>42</Newtype>` |`42` |
|Tuple |`<Tuple>42</Tuple><Tuple>answer</Tuple>` |`42 answer` |
|Struct |`<Struct><q>42</q><a>answer</a></Struct>`|Err(Unsupported)|
### New (since this commit)
|Kind |Top-level and in `$value` field |In normal field |In `$text` field|
|-------|-----------------------------------------|---------------------|----------------|
|Unit |`<Unit/>` |`<field>Unit</field>`|`Unit` |
|Newtype|`<Newtype>42</Newtype>` |Err(Unsupported) |Err(Unsupported)|
|Tuple |`<Tuple>42</Tuple><Tuple>answer</Tuple>` |Err(Unsupported) |Err(Unsupported)|
|Struct |`<Struct><q>42</q><a>answer</a></Struct>`|Err(Unsupported) |Err(Unsupported)|
`$text` enum variant
--------------------
`<field>` in top-level serialization is the name of top-level element, serialization is impossible if it is not defined in the serializer.
### Old (before this commit) representation
|Kind |In `$value` field |In normal field |In `$text` field|
|-------|-------------------------------|--------------------------|----------------|
|Unit |_(empty)_ |`<field/>` |_(empty)_ |
|Newtype|`42` |`<field>42</field>` |`42` |
|Tuple |`42 answer` |`<field>42 answer</field>`|`42 answer` |
|Struct |Err(Unsupported) |Err(Unsupported) |Err(Unsupported)|
### New (since this commit)
|Kind |Top-level and in `$value` field|In normal field |In `$text` field|
|-------|-------------------------------|--------------------------|----------------|
|Unit |_(empty)_ |`<field/>` |_(empty)_ |
|Newtype|`42` |Err(Unsupported) |Err(Unsupported)|
|Tuple |`42 answer` |Err(Unsupported) |Err(Unsupported)|
|Struct |Err(Unsupported) |Err(Unsupported) |Err(Unsupported)|
failures (28):
--lib (14)
se::element::tests::expand_empty_elements::enum_unit
se::element::tests::expand_empty_elements::enum_unit_escaped
se::element::tests::with_indent::attributes::enum_
se::element::tests::with_indent::enum_newtype
se::element::tests::with_indent::enum_struct
se::element::tests::with_indent::enum_tuple
se::element::tests::with_indent::enum_unit
se::element::tests::with_indent::enum_unit_escaped
se::element::tests::without_indent::attributes::enum_
se::element::tests::without_indent::enum_newtype
se::element::tests::without_indent::enum_struct
se::element::tests::without_indent::enum_tuple
se::element::tests::without_indent::enum_unit
se::element::tests::without_indent::enum_unit_escaped
serde-se (14):
without_root::enum_::externally_tagged::normal_field2::empty_struct
without_root::enum_::externally_tagged::normal_field2::newtype
without_root::enum_::externally_tagged::normal_field2::struct_
without_root::enum_::externally_tagged::normal_field2::tuple
without_root::enum_::externally_tagged::normal_field2::unit
without_root::enum_::externally_tagged::normal_field::empty_struct
without_root::enum_::externally_tagged::normal_field::newtype
without_root::enum_::externally_tagged::normal_field::struct_
without_root::enum_::externally_tagged::normal_field::tuple
without_root::enum_::externally_tagged::normal_field::unit
without_root::enum_::externally_tagged::text_variant::normal_field::newtype
without_root::enum_::externally_tagged::text_variant::normal_field::tuple
without_root::enum_::externally_tagged::text_variant::normal_field::unit
without_root::enum_::externally_tagged::text_variant::text_field::unit
Now only unit variants can be serialized, all other returns error
failures (4):
without_root::enum_::externally_tagged::normal_field2::unit
without_root::enum_::externally_tagged::normal_field::unit
without_root::enum_::externally_tagged::text_variant::normal_field::unit
without_root::enum_::externally_tagged::text_variant::text_field::unit
Fixed (24):
--lib (14, all)
se::element::tests::expand_empty_elements::enum_unit
se::element::tests::expand_empty_elements::enum_unit_escaped
se::element::tests::with_indent::attributes::enum_
se::element::tests::with_indent::enum_newtype
se::element::tests::with_indent::enum_struct
se::element::tests::with_indent::enum_tuple
se::element::tests::with_indent::enum_unit
se::element::tests::with_indent::enum_unit_escaped
se::element::tests::without_indent::attributes::enum_
se::element::tests::without_indent::enum_newtype
se::element::tests::without_indent::enum_struct
se::element::tests::without_indent::enum_tuple
se::element::tests::without_indent::enum_unit
se::element::tests::without_indent::enum_unit_escaped
serde-se (10):
without_root::enum_::externally_tagged::normal_field2::empty_struct
without_root::enum_::externally_tagged::normal_field2::newtype
without_root::enum_::externally_tagged::normal_field2::struct_
without_root::enum_::externally_tagged::normal_field2::tuple
without_root::enum_::externally_tagged::normal_field::empty_struct
without_root::enum_::externally_tagged::normal_field::newtype
without_root::enum_::externally_tagged::normal_field::struct_
without_root::enum_::externally_tagged::normal_field::tuple
without_root::enum_::externally_tagged::text_variant::normal_field::newtype
without_root::enum_::externally_tagged::text_variant::normal_field::tuple
failures (1):
without_root::enum_::externally_tagged::text_variant::text_field::unit
Fixed (5):
without_root::enum_::externally_tagged::normal_field2::unit
without_root::enum_::externally_tagged::normal_field::unit
without_root::enum_::externally_tagged::text_variant::normal_field::unit
Fixed (1):
without_root::enum_::externally_tagged::text_variant::text_field::uni
Collaborator
|
I don't have time to review this this week. Merge away |
Collaborator
Author
|
@dralley , heh, I not quite experienced in English slang. "Merge away" -- is this "ok, you can merge this, I anyway does not known serde's much" or "please don't merge, I want to do a review when I'll have time" 😅? |
Collaborator
|
It means you can merge it freely without needing my say so You can merge it, yes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR reworks our mapping of enums to XML. Actually, it is more correct, because examples in our documentation was not checked and not worked.
Summary of changes
Rules:
$valuefield the representation is always the same as top-level representation;$textfield the representation is always the same as in normal field, but surrounding tags with field name are removed;In
$textfields enums represented asxs:simpleTypeIn tables below the following type is used when I say about normal,
$textor$valuefields:Normal enum variant
Old
Before this PR the representation was:
$valuefields$textfield<Unit/><Newtype>42</Newtype>42<Tuple>42</Tuple><Tuple>answer</Tuple>42 answer<Struct><q>42</q><a>answer</a></Struct>New
In this PR the representation changed to:
$valuefield$textfield<Unit/><field>Unit</field>Unit<Newtype>42</Newtype><Tuple>42</Tuple><Tuple>answer</Tuple><Struct><q>42</q><a>answer</a></Struct>$textenum variant<field>in top-level serialization is the name of top-level element, serialization is impossible if it is not defined in the serializer.Old
Before this PR the representation was:
$textand$valuefields<field/><field>42</field>42<field>42 answer</field>42 answerNew
In this PR the representation changed to:
$valuefield$textfield<field/>4242 answerCloses #630
I think, this will be the last merged PR for this release cycle, because it seems that no one will be reviewed to the end of week. I plan to release 0.31.0 on weekend.
Footnotes
If this serialize as
<field>42</field>then it will be ambiguity during deserialization, because it clash withUnitrepresentation in normal field ↩If this serialize as
42then it will be ambiguity during deserialization, because it clash withUnitrepresentation in$textfield ↩If this serialize as
<field>42 answer</field>then it will be ambiguity during deserialization, because it clash withUnitrepresentation in normal field ↩If this serialize as
42 answerthen it will be ambiguity during deserialization, because it clash withUnitrepresentation in$textfield ↩