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

Added CDATA support to the toString() #23

Closed
wants to merge 3 commits into from
Closed

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Aug 6, 2015

For complex values that contain '<', '>', or '&',
support CDATA construct if opts.cdata is true.

Yuri Astrakhan added 3 commits August 6, 2015 18:39
For complex values that contain '<', '>', or '&',
support CDATA construct if opts.cdata is true.
* Attributes are not %-encoded, they should be escaped
  the same way as xml elements.
* The & must be escaped first, or else you double escape
  the elements with value like  "<" into "&amp;lt;"

http://www.liquid-technologies.com/XML/EscapingData.aspx
@amtrack
Copy link

amtrack commented Jan 25, 2016

👍

@nyurik
Copy link
Contributor Author

nyurik commented Jan 25, 2016

@nfarina, @wotzisname - is there anything wrong with this PR?

@nfarina
Copy link
Owner

nfarina commented Apr 27, 2016

Sorry it took me so long to review this!

I think this looks great for the most part. I like the new escapeValue function rather than the (probably misused) encodeURIComponent.

It looks like CDATA tags are output "automatically" when requested in options based on the presence of characters like <, > in the value. Would it be more correct to look at the isValCdata flag on XmlElement instead? That way we can preserve the format of the original XML structure.

@nyurik
Copy link
Contributor Author

nyurik commented Apr 27, 2016

@nfarina thanks for the review. For my usecase, I need to force various elements to cdata (silly mapnik lib has weird xml parsing rules). But for general use, sure, it might be good to allow both.

@nfarina
Copy link
Owner

nfarina commented Apr 27, 2016

You could control which elements are exported as CDATA by setting myElement.isValCdata = true on the ones you'd like - would that work for your use case?

@nyurik
Copy link
Contributor Author

nyurik commented Apr 27, 2016

might be a bit difficult - take a look - https://github.com/kartotherian/osm-bright.tm2source/blob/master/data.xml - for example, all params are cdata

@nfarina
Copy link
Owner

nfarina commented Apr 27, 2016

Are you generating this XML from scratch? Or parsing an existing document? If you're parsing an existing document, then isValCdata will already be set on the elements that came in as CDATA. If you're building it from scratch, then you would just set that property to true for all params.

@nyurik
Copy link
Contributor Author

nyurik commented Apr 27, 2016

i parse it in and alter a few values

@nfarina
Copy link
Owner

nfarina commented May 3, 2016

So when you parse CDATA tags does it correctly "round-trip" them back to the toString() function? If this works for you then I'd rather go with this method since it gives the developer control over the output. I find the idea of conditionally outputting CDATA based on content a bit strange - although I'd love feedback from other developers here.

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.

3 participants