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

Attribute value supports rml escape #154

Merged
merged 1 commit into from
Dec 11, 2020
Merged

Conversation

actboy168
Copy link
Contributor

This change allows escaping of element attributes. e.g.

<div data-style-decorator="'image(&quot;image.png&quot;)'" />

@mikke89
Copy link
Owner

mikke89 commented Dec 10, 2020

Thanks!

Some issues though:

  • The text comparisons are potentially out-of-bound accesses. They need to be guarded by the string size.
  • We can avoid an allocation by taking the input string by-value instead of const reference, and moving in the value parameter in BaseXmlParser. Then do the search and replace within the input string and return.

Also, I don't think we actually support quotes in the string property parser, it just consumes the whole thing. So you don't actually need the &quot; there as in your example. Which leads to the question if we really need this change?

@mikke89 mikke89 added the enhancement New feature or request label Dec 10, 2020
@actboy168
Copy link
Contributor Author

actboy168 commented Dec 11, 2020

  1. I think it will not be out-of-bound. Because the string always contains'\0' as the end, it will exit when it finds'\0'.
  2. I just want to be consistent with the function definition of EncodeRml. If it needs optimization, I can modify it.
  3. How do you think that my example should be implemented without &quot;?

@mikke89
Copy link
Owner

mikke89 commented Dec 11, 2020

  1. I had to double check with the specs, and it looks like you're right.
  2. Makes sense, I'll go over them both at some point.
  3. It should just be:
<div data-style-decorator="'image(image.png)'" />

I'll merge this, as there could be other use cases, and I consider this a more correct approach. Plus, I couldn't measure any performance degradation. So that's good :) Thanks!

@mikke89 mikke89 merged commit 5dfe9c4 into mikke89:master Dec 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants