Implemented change implementing "Keep specific element to single line while using indent with serde serializer#655" #814

RedIODev · 2024-10-01T14:23:11Z

Added checks to adjust indentation in case only attributes or empty elements are found.

Mingun

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.

Mingun · 2024-10-01T15:18:40Z

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "quick-xml"
-version = "0.36.2"
+version = "0.36.3"


In this project version is changed in special "release" commits, so please revert this change.

Suggested change

version = "0.36.3"

version = "0.36.2"

Mingun · 2024-10-01T15:21:01Z

src/se/element.rs

@@ -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,


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)

Suggested change

contains_non_attribute_keys:bool,

contains_non_attribute_keys: bool,

RedIODev · 2024-10-01T17:11:45Z

I will fix formatting, the version change and add the comment for the field. Edit: Also the changelog of cause
the tests:
se::element::tests::with_indent::value_field::map::enum_tuple
se::element::tests::with_indent::value_field::map::enum_unit
se::element::tests::with_indent::value_field::map::enum_struct
se::element::tests::with_indent::value_field::map::enum_newtype
se::element::tests::with_indent::value_field::map::tuple
se::element::tests::with_indent::value_field::map::tuple_struct

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
RedIODev

codecov-commenter · 2024-10-02T10:43:28Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.13%. Comparing base (39b5905) to head (01b6ccd).
Report is 1 commits behind head on master.

❗ 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

Flag	Coverage Δ
unittests	`60.13% <100.00%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Mingun · 2024-10-02T11:22:58Z

src/se/element.rs

+                //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)?;
+            }


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?

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

Anyway, it is not needed to write indent to get its length. You can directly ask self.ser.ser.indent about its length

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

Ok, I'll look then myself. Thanks for PR!

Mingun · 2024-10-11T17:31:42Z

I implemented alternative solution in #820. It handles all cases where text field may arise and does not require editing already written data. So I close this PR, but anyway thanks for submitting it!

RedIODev added 8 commits October 1, 2024 13:42

implent whitespace striping

7810efb

changed minor version

e814297

removed whitespace restoration

5f750f0

removed whitespace buffer

f19b3d7

added test print

4c8ead5

final fixes and cleanup

a8c5303

removed vscode settings

8dc04a4

removed unrelated changes and comments

1fcfcb1

Mingun requested changes Oct 1, 2024

View reviewed changes

Mingun linked an issue Oct 1, 2024 that may be closed by this pull request

Keep specific element to single line while using indent with serde serializer #655

Closed

RedIODev added 2 commits October 2, 2024 11:00

implemented requested changes

741eeaa

corrected changelog

01b6ccd

Mingun reviewed Oct 2, 2024

View reviewed changes

Mingun mentioned this pull request Oct 11, 2024

Do not write excess indents arount text fields when serialize with serde #820

Merged

Mingun closed this Oct 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented change implementing "Keep specific element to single line while using indent with serde serializer#655" #814

Implemented change implementing "Keep specific element to single line while using indent with serde serializer#655" #814

RedIODev commented Oct 1, 2024

Mingun left a comment

Mingun Oct 1, 2024

Mingun Oct 1, 2024

RedIODev commented Oct 1, 2024 •

edited

Loading

codecov-commenter commented Oct 2, 2024

Mingun Oct 2, 2024

RedIODev Oct 2, 2024

Mingun Oct 2, 2024

RedIODev Oct 2, 2024

Mingun Oct 3, 2024

Mingun commented Oct 11, 2024

	contains_non_attribute_keys:bool,
	contains_non_attribute_keys: bool,

Implemented change implementing "Keep specific element to single line while using indent with serde serializer#655" #814

Implemented change implementing "Keep specific element to single line while using indent with serde serializer#655" #814

Conversation

RedIODev commented Oct 1, 2024

Mingun left a comment

Choose a reason for hiding this comment

Mingun Oct 1, 2024

Choose a reason for hiding this comment

Mingun Oct 1, 2024

Choose a reason for hiding this comment

RedIODev commented Oct 1, 2024 • edited Loading

codecov-commenter commented Oct 2, 2024

Codecov Report

Mingun Oct 2, 2024

Choose a reason for hiding this comment

RedIODev Oct 2, 2024

Choose a reason for hiding this comment

Mingun Oct 2, 2024

Choose a reason for hiding this comment

RedIODev Oct 2, 2024

Choose a reason for hiding this comment

Mingun Oct 3, 2024

Choose a reason for hiding this comment

Mingun commented Oct 11, 2024

RedIODev commented Oct 1, 2024 •

edited

Loading