-
Notifications
You must be signed in to change notification settings - Fork 266
Fix serialization of structs with only text content as attributes #907
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
base: master
Are you sure you want to change the base?
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #907 +/- ##
==========================================
+ Coverage 55.52% 58.28% +2.75%
==========================================
Files 42 42
Lines 15511 15599 +88
==========================================
+ Hits 8613 9092 +479
+ Misses 6898 6507 -391
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:
|
|
I think, that makes sense. |
|
When polishing this PR I found, that such implementation creates inconsistency: you cannot parse struct back from the attribute value. That may be solved if consider Lines 124 to 138 in 655691c
but I prefer not to do that. As you known, some serde attributes may change struct representation to map representation and therefore we will lose our hack here, because maps does not report their keys. It seems, that in that case it is better to use Below the adapted test which failed even with this PR: /// Regression test for https://github.com/tafia/quick-xml/issues/906.
#[test]
fn issue906() {
#[derive(Debug, PartialEq, Deserialize, Serialize)]
struct AsElement {
#[serde(rename = "a-list")]
a_list: TextContent,
}
#[derive(Debug, PartialEq, Deserialize, Serialize)]
struct AsAttribute {
#[serde(rename = "@a-list")]
a_list: TextContent,
}
#[derive(Debug, PartialEq, Deserialize, Serialize)]
struct TextContent {
#[serde(rename = "$text")]
content: Vec<String>,
#[serde(skip)]
ignored: (),
}
let foo = AsElement {
a_list: TextContent {
content: vec!["A".to_string(), "B".to_string()],
ignored: (),
},
};
let bar = AsAttribute {
a_list: TextContent {
content: vec!["A".to_string(), "B".to_string()],
ignored: (),
},
};
let buffer = to_string_with_root("test", &foo).unwrap();
assert_eq!(buffer, "<test><a-list>A B</a-list></test>");
let foo2: AsElement = from_str(&buffer).unwrap();
assert_eq!(foo2, foo);
let buffer = to_string_with_root("test", &bar).unwrap();
assert_eq!(buffer, "<test a-list=\"A B\"/>");
// thread 'issue906' panicked at tests\serde-issues.rs:756:47:
// called `Result::unwrap()` on an `Err` value: Custom("invalid type: string \"A B\", expected struct TextContent")
let bar2: AsAttribute = from_str(&buffer).unwrap();
assert_eq!(bar2, bar);
}For element there is no problem, because |
|
Thanks for taking a look! It seems that I simply forgot about deserialization :) Writing custom (de)serializers always work, although I'd really like to see first party support for all usages of the text structs. Each struct would need their own implementation, and you'd have to track their usage and apply the with appropriately. Coming from code generators like https://docs.rs/xsd-parser/latest/xsd_parser/, this makes it very complicated IMHO. What do you think about the
Do you mean e.g. /// Regression test for https://github.com/tafia/quick-xml/issues/906.
#[test]
fn issue906() {
#[derive(Debug, PartialEq, Deserialize, Serialize)]
struct AsElement {
#[serde(rename = "a-list")]
a_list: TextContent,
}
#[derive(Debug, PartialEq, Deserialize, Serialize)]
pub struct TextContent {
#[serde(flatten)]
content: Inner,
}
#[derive(Debug, PartialEq, Deserialize, Serialize)]
pub struct Inner {
#[serde(default, rename = "$text")]
content: Vec<String>,
}
let foo = AsElement {
a_list: TextContent {
content: Inner {
content: vec!["A".to_string(), "B".to_string()],
},
},
};
let buffer = to_string_with_root("test", &foo).unwrap();
assert_eq!(buffer, "<test><a-list>A B</a-list></test>");
// thread 'with_root::list::issue906' panicked at tests/serde-se.rs:2347:53:
// called `Result::unwrap()` on an `Err` value: Custom("invalid type: string \"A B\", expected a sequence")
let foo2: AsElement = from_str(&buffer).unwrap();
assert_eq!(foo2, foo);
} |
|
I'm afraid the current approach requires a bump of serde to have your |
close #906
allows structs that contain only one $text field to be serialized as an attribute list.
As this is my first contribution, I may have overlooked something. The tests seem to pass though :)