-
Notifications
You must be signed in to change notification settings - Fork 267
WIP: Serde CData #849
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
WIP: Serde CData #849
Conversation
There was a problem hiding this 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.
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.
Adding tests and fixing escaping is not a big deal, it's more a question of how to idiomatically add this functionality |
Ok, if a real use case exist for that, that we can add You also need to update documentation that new |
Codecov ReportAttention: Patch coverage is
❗ 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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I apologize for bothering you. After a long correspondence, the software manufacturers acknowledged the problem and promised to solve it within a year. Then I will roll back to a temporary solution (waiting for a miracle), and close the PR |
@Mingun
I made some draft for $cdata serialization, deserialization the same as $text field, can you review?