Skip to content

openpyxl: type to_tree methods #10967

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

Merged
merged 11 commits into from
Dec 1, 2023
Merged

openpyxl: type to_tree methods #10967

merged 11 commits into from
Dec 1, 2023

Conversation

Avasam
Copy link
Collaborator

@Avasam Avasam commented Nov 2, 2023

Overlaps with #9511

One point of interest is that tagname parameter is sometimes passed directly to xml.etree.ElementTree.Element. But itself has the parameter typed as str | Callable[..., Element], which really doesn't make sense to me looking at usages, and there's no comment explaining why. It really seems like the tag parameter of Element.__init__ should not be Callable.

Also, even though Element.itertext does check for possible None self.tag, the docstring and documentation do suggest it should only be a str
Edit: see #10968

That being said, I may need to update some of my tagname parameters to not be possibly None in some/most scenarios.
Done

This comment has been minimized.

@Avasam Avasam changed the title Add Element to to_tree openpyxl: type to_tree methods Nov 2, 2023
@Avasam Avasam marked this pull request as ready for review November 21, 2023 20:27

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

github-actions bot commented Dec 1, 2023

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

Spot checking, LGTM

@srittau srittau merged commit 5f12eeb into python:main Dec 1, 2023
@Avasam Avasam deleted the openpyxl-to_tree branch December 1, 2023 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants