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

feat: generate OLX with single-quoted attributes (WIP) #35031

Closed

Conversation

kdmccormick
Copy link
Member

No description provided.

@kdmccormick
Copy link
Member Author

Since the latest iteration of the Content libraries upstream sync plan doesn't involve stuffing JSON into an OLX attribute, so I'm closing as this is no longer a priority.

However, if this is ever picked back up, here a summary of feedback from @ormsbee and @bradenmacdonald :

  • This would be good for OLX readability...
  • but it's a shame that there's builtin LXML option to do this for us...
  • although it's good that the logic here is at least isolated to one function in one module.
  • Having single quotes is not worth risking content integrity, so it would need to be airtight.
  • One way to make it failsafe would be to do a round-trip check when serializing, and fallback to the original double-quoted XML if it's not an exact match:
    xml_double_quotes = etree.tostring(root_node, ...)
    xml_single_quotes = "".join(_convert_attrs_to_single_quotes(iter(xml_str)))
    root_node_single_quotes_roundtripped = etree.fromstring(xml_single_quotes)
    if root_node_single_quotes_roundtripped == root_node:
        return xml_single_quotes
    else:
        return xml_double_quotes
  • Performance impacts of the above would need to be considered. Roll out with a waffle flag and monitor?

@kdmccormick kdmccormick closed this Sep 4, 2024
@kdmccormick kdmccormick deleted the kdmccormick/single-quote-xml branch September 4, 2024 16:39
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.

1 participant