Skip to content

Conversation

kkirsche
Copy link
Contributor

  1. encoding changed from Any | None to str | None as it's used as a parameter to TextIOWrapper
  2. standalone changed from Any | None to bool | None per https://docs.python.org/3/library/xml.dom.minidom.html#xml.dom.minidom.Node.writexml
  3. localName changed from Any | None to str | None as Node's localName property returns str | None
  4. Comparison operators set to object as they use id to do the comparisons, not object specific behavior: https://github.com/python/cpython/blob/main/Lib/xml/dom/minidom.py#L534
  5. TypeInfo partially typed based on https://cs.github.com/python/cpython/blob/5ed584cb6b4e544d307f2f1b6f28667078cc20af/Lib/xml/dom/expatbuilder.py?q=repo%3Apython%2Fcpython+path%3ALib%2Fxml%2F+TypeInfo#L47-L58
  6. get*Item check with a try / except: https://github.com/python/cpython/blob/main/Lib/xml/dom/minidom.py#L571
  7. Various create methods include isinstance type checking to force behaviors: https://github.com/python/cpython/blob/main/Lib/xml/dom/minidom.py#L1676

@kkirsche
Copy link
Contributor Author

Also, some of the parameter names of node are misleading, as the isinstance checks are checking for Attr nodes specifically, which is deceptive on a first pass:
https://github.com/python/cpython/blob/main/Lib/xml/dom/minidom.py#L607-L608

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks, I have a few comments.

@@ -70,7 +73,7 @@ class Attr(Node):
value: str
prefix: Any
def __init__(
self, qName: str, namespaceURI: str | None = ..., localName: Any | None = ..., prefix: Any | None = ...
self, qName: str, namespaceURI: str | None = ..., localName: str | None = ..., prefix: Any | None = ...
Copy link
Member

Choose a reason for hiding this comment

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

The localName argument is actually ignored: https://github.com/python/cpython/blob/18b1782192f85bd26db89f5bc850f8bee4247c1a/Lib/xml/dom/minidom.py#L355.

Seems like it wants a if localName is not None: self._localName = localName. Are you interested in contributing that to CPython?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'm happy to give that a shot.

Issue:
python/cpython#96175

PR:
python/cpython#96176

kkirsche added a commit to kkirsche/cpython that referenced this pull request Aug 22, 2022
@github-actions

This comment has been minimized.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

My comments were addressed, thanks. Still need to resolve @AlexWaygood's concern.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

I haven't reviewed in detail, but my concerns have been addressed, and I'll defer to @JelleZijlstra on the rest! :)

@github-actions

This comment has been minimized.

1 similar comment
@github-actions
Copy link
Contributor

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

@AlexWaygood AlexWaygood merged commit 269c075 into python:master Aug 22, 2022
@kkirsche kkirsche deleted the xml/dom-pulldom branch August 22, 2022 15:23
JelleZijlstra added a commit to python/cpython that referenced this pull request Aug 23, 2022
…Attr` (#96176)

X-Ref: python/typeshed#8590 (comment)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
mdboom pushed a commit to mdboom/cpython that referenced this pull request Aug 24, 2022
…nidom.Attr` (python#96176)

X-Ref: python/typeshed#8590 (comment)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants