bpo-34160: Preserve user specified order of Element attributes by rhettinger · Pull Request #10163

bpo-34160: Preserve user specified order of Element attributes by rhettinger · Pull Request #10163 · python/cpython

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove sorting also from other output methods (html, text). Add a What's New entry (in "Porting to 3.8").

Would you mind to remove sorting also from minidom output in this PR?

		_escape_attrib(v)
		))
		for k, v in sorted(items): # lexical order
		for k, v in items: # lexical order

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this warrants a whatsnew entry.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If other modules are to be changed as well (minidom, html modules, etc), it will need to be done in another PR. I don't have the spare time for those right now. Perhaps the other contributor on the tracker issue will continue to show an interest.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least remove sorting in other output methods in this module. And add tests for them.

		root = ET.Element('cirriculum', status='public', company='example')
		tree = ET.ElementTree(root)
		try:
		fo = open(support.TESTFN, "wb")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to use addCleanup(support.unlink, support.TESTFN) and with for working with files, or use io.StringIO. But it is better to use the serialize() helper.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what serialize() is. Switched from the traditional test support code to a context manager that does make it a little nicer.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

serialize() is a helper in this file which writes an XML to string/bytes/StringIO/BytesIO. It will allow you to use a single line instead of three or four lines.

self.assertEqual(serialize(tree),
                 '<cirriculum status="public" company="example" />')