Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

WIP: Serde CData #849

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

WIP: Serde CData #849

wants to merge 3 commits into from

Conversation

anton-dutov
Copy link

@Mingun
I made some draft for $cdata serialization, deserialization the same as $text field, can you review?

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.

I would prefer to not introduce $cdata rename, because serializing text as CDATA is a rather serializer choice instead of datatype choice. I already outlined my thoughts here.

Also, you implementation just wrong, because it not escaping ]]> in the serialized string and escapes &, < and >. The essential point of any CDATA-related PR should be test of those corner cases.

@anton-dutov
Copy link
Author

I would prefer to not introduce $cdata rename, because serializing text as CDATA is a rather serializer choice instead of datatype choice. I already outlined my thoughts here.

I understand your point, but unfortunately some software, e.g. implementing IATA CUPPS understand only CDATA in some elements, there are many of them and you can't change them. I can't explicitly tell the serializer that the data should be serialized as CDATA, so I'm looking for a solution. As I understand I am not the only one with this problem.

Also, you implementation just wrong, because it not escaping ]]> in the serialized string and escapes &, < and >. The essential point of any CDATA-related PR should be test of those corner cases.

Adding tests and fixing escaping is not a big deal, it's more a question of how to idiomatically add this functionality

@Mingun
Copy link
Collaborator

Mingun commented Mar 9, 2025

Ok, if a real use case exist for that, that we can add $cdata special rename in addition to the setting to the serializer. Would you like to implement variant with setting too?

You also need to update documentation that new $cdata rename is supported.

@codecov-commenter
Copy link

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

Codecov Report

Attention: Patch coverage is 14.40000% with 107 lines in your changes missing coverage. Please review.

Project coverage is 59.94%. Comparing base (a9391f3) to head (7f92441).
Report is 27 commits behind head on master.

Files with missing lines Patch % Lines
src/se/cdata.rs 5.30% 107 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #849      +/-   ##
==========================================
- Coverage   60.21%   59.94%   -0.27%     
==========================================
  Files          41       42       +1     
  Lines       16021    16142     +121     
==========================================
+ Hits         9647     9677      +30     
- Misses       6374     6465      +91     
Flag Coverage Δ
unittests 59.94% <14.40%> (-0.27%) ⬇️

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.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

Successfully merging this pull request may close these issues.

3 participants