Implemented change implementing "Keep specific element to single line while using indent with serde serializer#655" #814
Conversation
There was a problem hiding this comment.
CI will unhappy with some changes in whitespaces. Also, please update Changelog.md. Use issue number which is fixed (#655). Do not forgot that Changelog.md is a usual markdown file, so does not forgot to update section with links -- they are not linked automatically to the GH PRs/issues.
Cargo.toml
Outdated
| @@ -1,6 +1,6 @@ | |||
| [package] | |||
| name = "quick-xml" | |||
| version = "0.36.2" | |||
| version = "0.36.3" | |||
There was a problem hiding this comment.
In this project version is changed in special "release" commits, so please revert this change.
| version = "0.36.3" | |
| version = "0.36.2" |
src/se/element.rs
Outdated
| @@ -368,6 +370,7 @@ impl<'w, 'k, W: Write> SerializeTupleVariant for Tuple<'w, 'k, W> { | |||
| /// serializer | |||
| pub struct Struct<'w, 'k, W: Write> { | |||
| ser: ElementSerializer<'w, 'k, W>, | |||
| contains_non_attribute_keys:bool, | |||
There was a problem hiding this comment.
Could you add a bit of documentation, explaining how this field influence serialization (when it is true and when is false, what those values means)
| contains_non_attribute_keys:bool, | |
| contains_non_attribute_keys: bool, |
|
I will fix formatting, the version change and add the comment for the field. Edit: Also the changelog of cause are showing incorrect or questionable behavior, I or someone else should look into it before considering to merge this change. The rest of the tests would need to be changed to reflect the changed formatting. Rather you want to allow both behaviors using some toggle is something I have no part in but should be decided. Great regards |
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #814 +/- ##
==========================================
+ Coverage 60.08% 60.13% +0.04%
==========================================
Files 41 41
Lines 15975 15987 +12
==========================================
+ Hits 9599 9614 +15
+ Misses 6376 6373 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| //calculate indentation length | ||
| let mut indentation = String::new(); | ||
| self.ser.ser.indent.increase(); | ||
| self.ser.ser.indent.write_indent(&mut indentation)?; | ||
| self.ser.ser.indent.decrease(); | ||
|
|
||
| self.ser.ser.indent.write_indent(&mut self.ser.ser.writer)?; | ||
| let children = self.children.split_at(indentation.len()).1; | ||
|
|
||
| self.ser.ser.writer.write_str(children)?; | ||
| } |
There was a problem hiding this comment.
So the idea is to cut written indentation. I wonder, is it possible to defer intendation write until we will sure that it is necessary?
There was a problem hiding this comment.
That would require storing the serialized elements structured with their indentation separated instead of just the single string for all child elements.
I would guess the overhead of storing separated indentation and recombining all these for every struct would be quite large compared to removing indentation in 1 minor case.
I would guess more than 75% of structs use the other if branch that uses the normal behavior.
Since we only care about the case where the TEXT (<any>TEXT</any>) part is a single statement this was the easiest implementation I could think of. If you have a better idea to get the desired behavior feel free.
What I would suggest to take a second look is the new format for the tests mentioned above. It is questionable rather the formatting should be this way or rather they require a special case treatment.
Great regards
RedIODev
There was a problem hiding this comment.
Anyway, it is not needed to write indent to get its length. You can directly ask self.ser.ser.indent about its length
There was a problem hiding this comment.
I can't get it to work. The Indentation::current().len() and Indentation::current_indent_len both are off by 1 byte in some but not all cases. I can't see another way to ask Indent for its length.
Great regards
RedIODev
There was a problem hiding this comment.
Ok, I'll look then myself. Thanks for PR!
Added checks to adjust indentation in case only attributes or empty elements are found.