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

Implement BytesCData::escaped() fn #832

Merged
merged 1 commit into from
Nov 17, 2024
Merged

Conversation

Turbo87
Copy link
Contributor

@Turbo87 Turbo87 commented Nov 17, 2024

This PR should resolve #831 by implementing an escaped() fn on the BytesCData struct as suggested in #831 (comment).

I opted for keeping the new() fn around and introducing another function instead because the new() fn is used in various places in the quick-xml test suite and I didn't want to break anything. This should also allow us to ship this PR in a non-breaking version release since the existing new() fn keeps working as-is. We could consider deprecating the new() fn though to make it easier for users to understand that they should use escaped() instead. We could also add a raw() fn as an alias for new() if we would like to keep the behavior for some users while still deprecating new().

Related:

/cc @Mingun

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 for PR. Some minor improvements can be made here and there, and also please add changelog entry. Use link to the issue #831 in section for issues and brief description about added method in New Features section. See the other issues in the file for an example.

src/events/mod.rs Outdated Show resolved Hide resolved
src/events/mod.rs Outdated Show resolved Hide resolved
src/events/mod.rs Show resolved Hide resolved
src/events/mod.rs Show resolved Hide resolved
src/events/mod.rs Outdated Show resolved Hide resolved
src/events/mod.rs Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Nov 17, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 25 lines in your changes missing coverage. Please review.

Project coverage is 60.23%. Comparing base (a9391f3) to head (be09876).
Report is 17 commits behind head on master.

Files with missing lines Patch % Lines
src/events/mod.rs 0.00% 25 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #832      +/-   ##
==========================================
+ Coverage   60.21%   60.23%   +0.01%     
==========================================
  Files          41       41              
  Lines       16021    16029       +8     
==========================================
+ Hits         9647     9655       +8     
  Misses       6374     6374              
Flag Coverage Δ
unittests 60.23% <0.00%> (+0.01%) ⬆️

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.

@Turbo87
Copy link
Contributor Author

Turbo87 commented Nov 17, 2024

thanks for the quick review. I just pushed a couple of commits and I think I addressed all of the comments :)

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.

Good. Only add changelog entry please.

@Turbo87
Copy link
Contributor Author

Turbo87 commented Nov 17, 2024

Only add changelog entry please.

done!

@Mingun
Copy link
Collaborator

Mingun commented Nov 17, 2024

In the end I actually rewrite the whole implementation to use memchr. I think, it is now more straightforward and easy for understanding. But thanks anyway for your PR! The changes will be released soon in 0.37.1

@Mingun Mingun merged commit 6c4c766 into tafia:master Nov 17, 2024
7 checks passed
@Turbo87 Turbo87 deleted the cdata-escape branch November 17, 2024 21:26
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.

]]> in CDATA is not escaped
3 participants