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

xml: Add missing types for DocumentType's publicId and systemId #13343

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

JPTIZ
Copy link
Contributor

@JPTIZ JPTIZ commented Dec 30, 2024

Remarks:

  1. I've looked through the actual implementation and the usage of str | None should make sense. For the None part, the stdlib implementation use a few if obj.publicId:, see for example:
    $ grep -n -R publicId  /usr/lib/python3.12/xml/dom/
    /usr/lib/python3.12/xml/dom/xmlbuilder.py:215:    def resolveEntity(self, publicId, systemId):
    /usr/lib/python3.12/xml/dom/xmlbuilder.py:218:        source.publicId = publicId
    /usr/lib/python3.12/xml/dom/xmlbuilder.py:258:                 'encoding', 'publicId', 'systemId', 'baseURI')
    /usr/lib/python3.12/xml/dom/xmlbuilder.py:265:        self.publicId = None
    /usr/lib/python3.12/xml/dom/xmlbuilder.py:289:    def _get_publicId(self):
    /usr/lib/python3.12/xml/dom/xmlbuilder.py:290:        return self.publicId
    /usr/lib/python3.12/xml/dom/xmlbuilder.py:291:    def _set_publicId(self, publicId):
    /usr/lib/python3.12/xml/dom/xmlbuilder.py:292:        self.publicId = publicId
    /usr/lib/python3.12/xml/dom/minidom.py:1294:    """Mix-in class that supports the publicId and systemId attributes."""
    /usr/lib/python3.12/xml/dom/minidom.py:1296:    __slots__ = 'publicId', 'systemId'
    /usr/lib/python3.12/xml/dom/minidom.py:1298:    def _identified_mixin_init(self, publicId, systemId):
    /usr/lib/python3.12/xml/dom/minidom.py:1299:        self.publicId = publicId
    /usr/lib/python3.12/xml/dom/minidom.py:1302:    def _get_publicId(self):
    /usr/lib/python3.12/xml/dom/minidom.py:1303:        return self.publicId
    /usr/lib/python3.12/xml/dom/minidom.py:1312:    publicId = None
    /usr/lib/python3.12/xml/dom/minidom.py:1338:                    notation = Notation(n.nodeName, n.publicId, n.systemId)
    /usr/lib/python3.12/xml/dom/minidom.py:1342:                    entity = Entity(e.nodeName, e.publicId, e.systemId,
    /usr/lib/python3.12/xml/dom/minidom.py:1357:        if self.publicId:
    /usr/lib/python3.12/xml/dom/minidom.py:1359:                         % (newl, self.publicId, newl, self.systemId))
    /usr/lib/python3.12/xml/dom/minidom.py:1377:    def __init__(self, name, publicId, systemId, notation):
    /usr/lib/python3.12/xml/dom/minidom.py:1381:        self._identified_mixin_init(publicId, systemId)
    /usr/lib/python3.12/xml/dom/minidom.py:1412:    def __init__(self, name, publicId, systemId):
    /usr/lib/python3.12/xml/dom/minidom.py:1414:        self._identified_mixin_init(publicId, systemId)
    /usr/lib/python3.12/xml/dom/minidom.py:1478:    def createDocumentType(self, qualifiedName, publicId, systemId):
    /usr/lib/python3.12/xml/dom/minidom.py:1480:        doctype.publicId = publicId
    /usr/lib/python3.12/xml/dom/minidom.py:1731:    def _create_entity(self, name, publicId, systemId, notationName):
    /usr/lib/python3.12/xml/dom/minidom.py:1732:        e = Entity(name, publicId, systemId, notationName)
    /usr/lib/python3.12/xml/dom/minidom.py:1736:    def _create_notation(self, name, publicId, systemId):
    /usr/lib/python3.12/xml/dom/minidom.py:1737:        n = Notation(name, publicId, systemId)
    /usr/lib/python3.12/xml/dom/minidom.py:1936:            node.name, node.publicId, node.systemId)
    /usr/lib/python3.12/xml/dom/minidom.py:1942:                notation = Notation(n.nodeName, n.publicId, n.systemId)
    /usr/lib/python3.12/xml/dom/minidom.py:1948:                entity = Entity(e.nodeName, e.publicId, e.systemId,
    /usr/lib/python3.12/xml/dom/expatbuilder.py:237:    def start_doctype_decl_handler(self, doctypeName, systemId, publicId,
    /usr/lib/python3.12/xml/dom/expatbuilder.py:240:            doctypeName, publicId, systemId)
    /usr/lib/python3.12/xml/dom/expatbuilder.py:303:                            base, systemId, publicId, notationName):
    /usr/lib/python3.12/xml/dom/expatbuilder.py:309:        node = self.document._create_entity(entityName, publicId,
    /usr/lib/python3.12/xml/dom/expatbuilder.py:320:    def notation_decl_handler(self, notationName, base, systemId, publicId):
    /usr/lib/python3.12/xml/dom/expatbuilder.py:321:        node = self.document._create_notation(notationName, publicId, systemId)
    /usr/lib/python3.12/xml/dom/expatbuilder.py:340:    def external_entity_ref_handler(self, context, base, systemId, publicId):
    /usr/lib/python3.12/xml/dom/expatbuilder.py:627:            if doctype.publicId:
    /usr/lib/python3.12/xml/dom/expatbuilder.py:629:                         % (doctype.publicId, doctype.systemId))
    /usr/lib/python3.12/xml/dom/expatbuilder.py:660:                if notation.publicId:
    /usr/lib/python3.12/xml/dom/expatbuilder.py:662:                        % (s, notation.publicId, notation.systemId)
    /usr/lib/python3.12/xml/dom/expatbuilder.py:670:                if entity.publicId:
    /usr/lib/python3.12/xml/dom/expatbuilder.py:672:                        % (s, entity.publicId, entity.systemId)
    /usr/lib/python3.12/xml/dom/expatbuilder.py:685:    def external_entity_ref_handler(self, context, base, systemId, publicId):
    /usr/lib/python3.12/xml/dom/expatbuilder.py:705:                self, context, base, systemId, publicId)
    /usr/lib/python3.12/xml/dom/expatbuilder.py:877:    def start_doctype_decl_handler(self, name, publicId, systemId,
  2. The str | None types can also be infered from DocumentType docs
  3. The DomImplementation.createDocumentType method in stdlib implementation does a straight-forward assignment:
        def createDocumentType(self, qualifiedName, publicId, systemId):
            doctype = DocumentType(qualifiedName)
            doctype.publicId = publicId
            doctype.systemId = systemId
            return doctype
    So, changing the publicId and systemId parameter types to str | None should make sense as well.

Copy link
Contributor

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

@tungol
Copy link
Contributor

tungol commented Jan 12, 2025

I pushed up a bigger xml MR within 24 hours of this one, funny enough. I just checked and I came to the same conclusion in both of these places in #13349, so +1 from me here.

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.

Thanks!

@srittau srittau merged commit 1017916 into python:main Jan 13, 2025
64 checks passed
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.

3 participants