-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
bpo-37374: Do not escape quotes in minidom inside text segments #14312
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
Conversation
Hi and thank you for the pull request. |
Yes, I tested by running: from xml.etree import ElementTree
text = ElementTree.Element('text')
text.text = 'f&oo"b<a>r'
xml_string = ElementTree.tostring(text)
import xml.dom.minidom as minidom
xml_tree = minidom.parseString(xml_string)
output = xml_tree.toprettyxml(indent=' ')
assert output.splitlines()[1] == xml_string.decode('utf8')
print(output) which now returns:
I also ran whole Python test suite. |
Perhaps you should provide it in the test_xml module. |
You are right. I should add the test. This PR in fact makes minidom behave exactly the same as |
df9c8b1
to
393923f
Compare
I have added tests. |
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.
Thank you for the PR. However, I've rejected the ticket, so I have to reject the PR, too.
I'll leave my review comments anyway, in case it gets reconsidered at some later point.
def testQuoteEscape(self): | ||
text = ElementTree.Element('text') | ||
text.text = 'f&oo"b<a>r' | ||
xml_string = ElementTree.tostring(text) |
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.
We should not rely on an unrelated library providing the same text string serialisation. Instead, the test should use a plain string to compare against. Otherwise, changes in ElementTree could make this test fail without need.
def writexml(self, writer, indent="", addindent="", newl=""): | ||
_write_data(writer, "%s%s%s" % (indent, self.data, newl)) | ||
_write_text_data(writer, "%s%s%s" % (indent, self.data, newl)) |
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 haven't checked deeply, but my gut feeling would prefer a special case for attribute values, not for text content. Can't say if that's similarly easy to achieve.
Addressing the issue: https://bugs.python.org/issue37374
https://bugs.python.org/issue37374