Skip to content
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

Add expand_empty_elements fn to Serializer #620

Merged
merged 5 commits into from
Jun 30, 2023

Conversation

bahlo
Copy link
Contributor

@bahlo bahlo commented Jun 27, 2023

Setting this to true will serialize empty elements to <element></element> instead of <element/>.

Fixes #617

Setting this to true will serialize empty elements to
<element></element> instead of <element/>.
Copy link
Collaborator

@Mingun Mingun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add changelog entry (do not forgot to add a link target, it in the end of corresponding release section).

Other than that and a minor note about documentation looks good.

src/se/mod.rs Outdated
Comment on lines 532 to 536
/// Enable or disable expansion of empty elements.
pub fn expand_empty_elements(&mut self, expand: bool) -> &mut Self {
self.ser.expand_empty_elements = expand;
self
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a doctest example and specify that default value is false.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mingun I added a dockest and noticed changing the expand_empty_elements in the method does not affect the ContentSerializer that actually does the serializing.

I didn't find the culprit yet, is this due to macros or cloning?

@codecov-commenter
Copy link

Codecov Report

Merging #620 (1f44bb4) into master (60249ae) will increase coverage by 0.00%.
The diff coverage is 88.57%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##           master     #620   +/-   ##
=======================================
  Coverage   64.67%   64.68%           
=======================================
  Files          36       36           
  Lines       16998    17032   +34     
=======================================
+ Hits        10994    11017   +23     
- Misses       6004     6015   +11     
Flag Coverage Δ
unittests 64.68% <88.57%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
src/se/mod.rs 90.61% <33.33%> (-1.67%) ⬇️
src/se/content.rs 97.26% <100.00%> (+0.03%) ⬆️
src/se/element.rs 98.53% <100.00%> (+0.02%) ⬆️

... and 4 files with indirect coverage changes

Copy link
Collaborator

@Mingun Mingun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! One minor note about link and everything other is good

Changelog.md Outdated Show resolved Hide resolved
@Mingun Mingun merged commit 05059fd into tafia:master Jun 30, 2023
@bahlo
Copy link
Contributor Author

bahlo commented Jun 30, 2023

@Mingun Ooops, I don't think this should've been merged, tests are failling.

See my comment above:

I added a dockest and noticed changing the expand_empty_elements in the method does not affect the ContentSerializer that actually does the serializing.
I didn't find the culprit yet, is this due to macros or cloning?

Mingun added a commit to Mingun/quick-xml that referenced this pull request Jun 30, 2023
@Mingun
Copy link
Collaborator

Mingun commented Jun 30, 2023

Oops... I thought I saw the green OK, but forgot, that I did not wait for the tests to be completed last night. Fixed in 3ab7f64. Also strange, that GitHub did not highlight your comment for me

@bahlo
Copy link
Contributor Author

bahlo commented Jun 30, 2023

Thanks for fixing it @Mingun 🙏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

quick-xml v0.27+ changed behaviour when writing empty values
3 participants