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.

Sign up for GitHub

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

Jump to bottom

bpo-34160: Preserves order of minidom of Element attributes #10219

Merged
merged 3 commits into from
Nov 7, 2018
Merged

bpo-34160: Preserves order of minidom of Element attributes #10219

merged 3 commits into from
Nov 7, 2018

Conversation

dfrojas
Copy link
Contributor

@dfrojas dfrojas commented Oct 29, 2018 •
edited
Loading

This PR is a continuation of the work in #10163

https://bugs.python.org/issue34160

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

vstinner
vstinner reviewed Oct 29, 2018
View reviewed changes
Lib/xml/etree/ElementTree.py Outdated
@@ -979,7 +979,7 @@ def _serialize_html(write, elem, qnames, namespaces, **kwargs):
k,
_escape_attrib(v)
))
for k, v in sorted(items): # lexical order
for k, v in items:
Copy link
Member

Choose a reason for hiding this comment

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

This change has already been committed: commit 3b05ad7, PR #10190.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, I did not realize that @serhiy-storchaka have made that changes some minutes before this PR.

vstinner
vstinner reviewed Oct 29, 2018
View reviewed changes
Lib/xml/dom/minidom.py Outdated
@@ -854,7 +854,7 @@ def writexml(self, writer, indent="", addindent="", newl=""):
writer.write(indent+"<" + self.tagName)

attrs = self._get_attributes()
a_names = sorted(attrs.keys())
a_names = attrs.keys()

for a_name in a_names:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for a_name in a_names:
for a_name in attrs.keys():

vstinner
vstinner reviewed Oct 29, 2018
View reviewed changes
Doc/library/xml.etree.elementtree.rst Outdated
@@ -642,6 +642,13 @@ Functions
:class:`Element` instance and a dictionary.


.. function:: _serialize_html(self, writer, indent="", addindent="", newl="")
Copy link
Member

Choose a reason for hiding this comment

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

No, please don't document private functions.

serhiy-storchaka
serhiy-storchaka reviewed Oct 29, 2018
View reviewed changes
@dfrojas dfrojas requested review from 1st1 and asvetlov as code owners October 30, 2018 03:21
@dfrojas dfrojas requested a review from a team October 30, 2018 03:21
@dfrojas
Copy link
Contributor Author

dfrojas commented Oct 30, 2018

@vstinner @serhiy-storchaka I had conflicts doing the changes, the PR has commits from other developers. It's better if I close this PR and open a new one?

@serhiy-storchaka
Copy link
Member

Something wrong happen when you synchronized your branch with master. Did you run git merge master? If there are conflicts, it is better to resolve them and continue in the same PR.

@taleinat
Copy link
Contributor

taleinat commented Oct 30, 2018 •
edited by bedevere-bot
Loading

@dfrojas, I recommend:

  1. Create a new branch from the current master branch.
  2. Cherry-pick your relevant commits one by one in chronological order.
  3. Review the changes, run the tests etc.
  4. If everything looks okay, force-push the new branch into the branch for this PR, e.g. on the new branch: git push -f origin [bpo-34160](https://bugs.python.org/issue34160)-10163PR-continuation

@dfrojas dfrojas changed the title bpo-34160: Preserves order of minidom and html serialize of Element attributes bpo-34160: Preserves order of minidom of Element attributes Oct 30, 2018
serhiy-storchaka
serhiy-storchaka reviewed Oct 30, 2018
View reviewed changes
@dfrojas
Copy link
Contributor Author

dfrojas commented Oct 30, 2018

@serhiy-storchaka Do I have to write a NEWS specifically for minidom? Or with the NEWS in this PR is enough?

@taleinat
Copy link
Contributor

taleinat commented Nov 2, 2018

@serhiy-storchaka Do I have to write a NEWS specifically for minidom? Or with the NEWS in this PR is enough?

Perhaps just update the NEWS entry you referenced and add "minidom" after "ElementTree".

@serhiy-storchaka
Copy link
Member

Either add a new news entry, or update an existing entry added for #10219. The rest LGTM.

@serhiy-storchaka serhiy-storchaka added the type-feature A feature request or enhancement label Nov 7, 2018
@serhiy-storchaka serhiy-storchaka merged commit 5598cc9 into python:master Nov 7, 2018
@jayaddison jayaddison mentioned this pull request Dec 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner left review comments

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@1st1 1st1 Awaiting requested review from 1st1

@asvetlov asvetlov Awaiting requested review from asvetlov

Assignees
No one assigned
Labels
type-feature A feature request or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dfrojas @the-knights-who-say-ni @serhiy-storchaka @taleinat @vstinner @bedevere-bot