bpo-34160: Make sorting attributes in XML serialization optional.#10452
bpo-34160: Make sorting attributes in XML serialization optional.#10452serhiy-storchaka wants to merge 5 commits intopython:masterfrom
Conversation
|
Wow, that was fast! :) |
| @@ -1 +1,2 @@ | |||
| ElementTree and minidom now preserve the attribute order specified by the user. | |||
| Added support for the *sort_attrs* argument in methods for serializing XML in | |||
| modules :mod:`xml.etree.ElementTree` and :mod:`xml.dom.minidom`. | |||
There was a problem hiding this comment.
I think it makes sense to note both changes: 1) now by default the attribute order is preserved, and 2) there's an option to sort them.
There was a problem hiding this comment.
No, the default was not changed. I think it is better first add an option, and change the default in next the version. This will make supporting two consequent versions (3.7-3.8 or 3.8-3.9) easier.
Mariatta
left a comment
There was a problem hiding this comment.
Some comments about the documentation. I'm actually unsure about some of these..
| For the :class:`Document` node, an additional keyword argument *encoding* can | ||
| be used to specify the encoding field of the XML header. | ||
|
|
||
| If *sort_attrs* is true (by default), then element attributes will be |
There was a problem hiding this comment.
| If *sort_attrs* is true (by default), then element attributes will be | |
| If *sort_attrs* is ``True`` (the default value), then element attributes will be |
There was a problem hiding this comment.
No, the current wording is more correct. Element attributes will be sorted not only when sort_attrs is True, but when its boolean value is true.
"True" is a concrete bool instance, "true" is used when the boolean value should be true, and not just bool instances are accepted.
| of elements that contain no content. If ``True`` (the default), they are | ||
| emitted as a single self-closed tag, otherwise they are emitted as a pair | ||
| of start/end tags. | ||
| If *sort_attrs* is true (by default), then element attributes will be |
There was a problem hiding this comment.
| If *sort_attrs* is true (by default), then element attributes will be | |
| If *sort_attrs* is ``True`` (the default value), then element attributes will be |
Co-Authored-By: serhiy-storchaka <[email protected]>
Co-Authored-By: serhiy-storchaka <[email protected]>
Co-Authored-By: serhiy-storchaka <[email protected]>
|
Thank you for your review Mariatta. Although this PR was purposed for demonstration. It should not be merged as is. |
|
I would like to vote against this much API expansion. We will have to live it forever. IMO, we ought to have a single canonicalization tool and everything else should just be order preserving. AFAICT, the only original reason for having sorting was to produce a deterministic order, not because anyone has ever desired alphabetical ordering. From a user's point of view, there would almost never be a reason to set sort_attrs=True, so being presented with the option would just be noise. FWIW, at least twice a month, I teach people how to use these APIs and it is already an uphill battle to work through all the existing options. We should be careful with API expansion, because it becomes a permanent promise that is difficult to take away. Each option becomes yet another thing to learn and remember. I did a quick scan of XML APIs in other languages (such as Go) and did not see a sort_attrs option. That should be an indicator that this isn't an essential feature in this problem domain. The existence of canonicalization tools does indicate how the problem is approached elsewhere. AFAICT, for applications to test XML generation, the best practices are either to verify that the XML round-trips (like when we test |
|
Rejected, as discussed in the ticket. |
https://bugs.python.org/issue34160