Skip to content

Simplify xml.etree.ElementTree loading#902

Merged
waylan merged 6 commits intoPython-Markdown:masterfrom
mitya57:simplify-etree-import
Feb 3, 2020
Merged

Simplify xml.etree.ElementTree loading#902
waylan merged 6 commits intoPython-Markdown:masterfrom
mitya57:simplify-etree-import

Conversation

@mitya57
Copy link
Copy Markdown
Collaborator

@mitya57 mitya57 commented Feb 1, 2020

cElementTree is a deprecated alias for ElementTree since Python 3.3. As we are now supporting only Python ≥ 3.5, we do not need to use it.

The version check is also no longer needed. The module has version 1.3.0 since 2010, so even Python 2.7 and 3.2 have it (see python/cpython@f15351d938e76c4b).

Copy link
Copy Markdown
Member

@waylan waylan left a comment

Choose a reason for hiding this comment

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

Simplifying the code by removing old cruft like this is great. However, the cruft has only been half removed. See my comments below for what need sot happen before this can be accepted.

Comment thread markdown/util.py
Comment thread markdown/util.py Outdated
Comment thread markdown/util.py Outdated
@waylan waylan added core Related to the core parser code. requires-changes Awaiting updates after a review. labels Feb 1, 2020
@waylan waylan added this to the Version 3.2 milestone Feb 1, 2020
cElementTree is a deprecated alias for ElementTree since Python 3.3.

Also drop the recommendation to import etree from markdown.util,
and deprecate markdown.util.etree.
@mitya57 mitya57 force-pushed the simplify-etree-import branch from 7c4f523 to 0f48d17 Compare February 1, 2020 21:19
@mitya57
Copy link
Copy Markdown
Collaborator Author

mitya57 commented Feb 1, 2020

Now that we deprecated this, I had to update a lot of internal code. Commit 0f48d17 deprecates the old markdown.util.etree wrapper, commits 80729a3 and 8b1f49a make the replacements, and commit 3b7a55b updates the documentation.

Copy link
Copy Markdown
Member

@waylan waylan left a comment

Choose a reason for hiding this comment

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

This looks good. However, one thing is bothering me. Sometimes we import as etree and other times we don't do the as etree bit. Lets be consistent throughout the lib.

@mitya57 mitya57 force-pushed the simplify-etree-import branch from 8b1f49a to 03c21fb Compare February 2, 2020 08:50
@mitya57
Copy link
Copy Markdown
Collaborator Author

mitya57 commented Feb 2, 2020

Ok. I decided to go with the as etree form as it is shorter and requires less changes throughout the code.

@waylan
Copy link
Copy Markdown
Member

waylan commented Feb 2, 2020

We should probably add a note to the release notes informing extension devs that markdown.util.etree is deprecated and that they should import xml.etree.ElementTree directly. With that small change, I think this is ready to go. Nice work.

@mitya57
Copy link
Copy Markdown
Collaborator Author

mitya57 commented Feb 2, 2020

Done in 01ca739. Please check if the wording is fine.

@waylan
Copy link
Copy Markdown
Member

waylan commented Feb 3, 2020

Please check if the wording is fine.

It was pretty good. I made a few adjustments to the first sentence for consistency ("some.module.path module" requires the article "the" but if you leave out the word "module" then you also should leave out the article). The other changes were mostly additions which I probably wouldn't have bothered with if I wasn't already making changes.

@waylan waylan merged commit a2e4788 into Python-Markdown:master Feb 3, 2020
mitya57 added a commit to mitya57/python-markdown-math that referenced this pull request Feb 7, 2020
kernc added a commit to pdoc3/pdoc that referenced this pull request Feb 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Related to the core parser code. requires-changes Awaiting updates after a review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants