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.

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

Add types for defusedxml #11179

Merged
merged 1 commit into from
Jan 14, 2024
Merged

Add types for defusedxml #11179

merged 1 commit into from
Jan 14, 2024

Conversation

solkaz
Copy link
Contributor

@solkaz solkaz commented Dec 18, 2023

Initial attempt at adding types for defusedxml.

Some notes:

  • This library relies on modules that aren't typed; namely:
    • xml.sax.expatreader - undocumented
    • lxml - no types, and I would prefer to ignore it for now

Comment on lines 1 to 11
from .ElementTree import (
DefusedXMLParser,
ParseError as ParseError,
fromstring as fromstring,
iterparse as iterparse,
parse as parse,
tostring as tostring,
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original source code has the following imports:

XMLTreeBuilder as XMLTreeBuilder
XMLParse as XMLParse
XMLParser as XMLParser
XML as XML

They were removed as Flake8 complained that below, they were being redefined, or that XML could not be redefined because it's a constant. If this isn't right, let me know how I could improve this

Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI: The current version of this file looks good, although you should probably add the __all__ statements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srittau added

This comment has been minimized.

@solkaz solkaz force-pushed the defusedxml branch 2 times, most recently from 6a721a7 to b6ca295 Compare December 21, 2023 21:24

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@solkaz solkaz marked this pull request as ready for review December 28, 2023 12:50
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

vision (https://github.com/pytorch/vision)
+ torchvision/datasets/voc.py:10: error: Incompatible import of "ET_parse" (imported name has type "Callable[[int | str | bytes | PathLike[str] | PathLike[bytes] | SupportsRead[bytes] | SupportsRead[str], XMLParser | None], ElementTree]", local name has type "Callable[[Any, DefusedXMLParser | None, bool, bool, bool], ElementTree]")  [assignment]

@solkaz solkaz requested a review from srittau December 28, 2023 13:59
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.

I'm sorry for the late review. This looks good to me, although I didn't compare it to the implementation.

@srittau srittau merged commit 0b43c1d into python:main Jan 14, 2024
48 checks passed
@solkaz
Copy link
Contributor Author

solkaz commented Jan 15, 2024

@srittau thank you for the review! I'll follow up with fixes once I implement this into my personal project

@jbethune
Copy link
Contributor

@solkaz Thanks for putting this together!

I noticed that defusedxml.minidom.parse and defusedxml.minidom.parseString don't have return type annotations. The return type should be xml.dom.minidom.Document. See also: expatbuilder.parse()

I know this PR is already merged. I guess we need to create a new one?

@srittau
Copy link
Collaborator

srittau commented Jan 16, 2024

I know this PR is already merged. I guess we need to create a new one?

Indeed. Any PRs to further improve the stubs are welcome!

@tyrossel
Copy link
Contributor

tyrossel commented Jan 24, 2024

Thanks for adding those types !

I got an error with the find function:

import xml.etree.ElementTree as ET
import defusedxml.ElementTree as DET

el = DET.fromstring(filecontent)
subEl = el.find("Document", ns)

Mypy gives the error: mypy: "Element" has no attribute "find" (i.e. xml.dom.minidom.Element)

I can cast el to ET.Element to solve this issue, but I had the understanding that defusedxml was a drop-in replacement to those functions.

I am not sure where I should report this, let me know if there is a better place.

@AlexWaygood
Copy link
Member

@tyrossel, please open an issue on our tracker if there's a problem with these stubs; it's hard to keep track of comments on merged PRs :)

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.

5 participants