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

fix: add missing type stubs for xml.dom.minidom.DOMImplementation, where possible #8556

Merged
merged 3 commits into from
Aug 20, 2022
Merged

Conversation

kkirsche
Copy link
Contributor

This adds additional details to xml.dom.minidom.DOMImplementation based on what I am able to determine from:
https://github.com/python/cpython/blob/main/Lib/xml/dom/minidom.py

This begins to address #6886

@github-actions

This comment has been minimized.

def createDocumentType(self, qualifiedName: str | None, publicId, systemId): ...
def getInterface(self, feature): ...
def hasFeature(self, feature: str, version: str | None) -> bool: ...
def createDocument(self, namespaceURI: str | None, qualifiedName: str | None, doctype: DocumentType | None) -> Document: ...
Copy link
Member

Choose a reason for hiding this comment

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

I couldn't find definitive evidence for this in the source code, but the documentation for this method confirms this type annotation. Looks good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had been trying to use https://cs.github.com/?scopeName=All+repos&scope=&q=repo%3Apython%2Fcpython+path%3ALib%2Fxml%2Fdom+%22.createDocument%28%22 to work backwards through what calls were being used and comparing it to the documentation as my way of trying to find evidence, if that helps

Copy link
Member

Choose a reason for hiding this comment

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

Oh, that is helpful, thanks! Especially with xml, it's sometimes pretty difficult to figure out the types just by studying the source code, so hints as to how you figured out non-obvious types are definitely appreciated :)

stdlib/xml/dom/minidom.pyi Outdated Show resolved Hide resolved
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.

Nice! Always great to get better types for our xml stubs — I think we probably all agree it's the weakest area in our stdlib stubs at the moment :)

@github-actions
Copy link
Contributor

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

@AlexWaygood AlexWaygood merged commit fb62cca into python:master Aug 20, 2022
@kkirsche
Copy link
Contributor Author

Nice! Always great to get better types for our xml stub

For whatever reason, security standards like OpenSCAP often default to XML standards so I end up in either xml or xsdata more often than I'd like. I'll need to look at what's there and what's missing to see what's involved with bringing it more inline with other areas of the standard library, in terms of coverage.

@kkirsche kkirsche deleted the fix/6886 branch August 20, 2022 19:51
@AlexWaygood
Copy link
Member

AlexWaygood commented Aug 20, 2022

You can see here that there are a number of xml files that we have to exclude from the stricter pyright settings in our CI:

"exclude": [
"stdlib/distutils/command",
"stdlib/lib2to3/refactor.pyi",
"stdlib/multiprocessing/reduction.pyi",
"stdlib/sqlite3/dbapi2.pyi",
"stdlib/_tkinter.pyi",
"stdlib/tkinter/__init__.pyi",
"stdlib/tkinter/colorchooser.pyi",
"stdlib/tkinter/commondialog.pyi",
"stdlib/tkinter/dialog.pyi",
"stdlib/tkinter/dnd.pyi",
"stdlib/tkinter/filedialog.pyi",
"stdlib/tkinter/messagebox.pyi",
"stdlib/tkinter/scrolledtext.pyi",
"stdlib/tkinter/tix.pyi",
"stdlib/tkinter/ttk.pyi",
"stdlib/xml/dom/NodeFilter.pyi",
"stdlib/xml/dom/expatbuilder.pyi",
"stdlib/xml/dom/minidom.pyi",
"stdlib/xml/dom/pulldom.pyi",
"stdlib/xml/sax",
"stubs/aiofiles/aiofiles/tempfile/temptypes.pyi",
"stubs/aiofiles/aiofiles/threadpool/utils.pyi",
"stubs/annoy/annoy/annoylib.pyi",
"stubs/aws-xray-sdk",
"stubs/babel",
"stubs/backports.ssl_match_hostname",
"stubs/bleach",
"stubs/boto",
"stubs/beautifulsoup4",
"stubs/braintree",
"stubs/caldav",
"stubs/commonmark",
"stubs/cryptography",
"stubs/dateparser",
"stubs/dj-database-url",
"stubs/docutils",
"stubs/editdistance/editdistance/bycython.pyi",
"stubs/Flask-SQLAlchemy",
"stubs/fpdf2",
"stubs/html5lib",
"stubs/httplib2",
"stubs/humanfriendly",
"stubs/invoke",
"stubs/jmespath",
"stubs/jsonschema",
"stubs/ldap3",
"stubs/Markdown",
"stubs/mock",
"stubs/mysqlclient",
"stubs/oauthlib",
"stubs/openpyxl",
"stubs/Pillow",
"stubs/paramiko",
"stubs/prettytable",
"stubs/protobuf",
"stubs/pytz/pytz/lazy.pyi",
"stubs/pytz/pytz/reference.pyi",
"stubs/pytz/pytz/tzfile.pyi",
"stubs/google-cloud-ndb",
"stubs/passlib",
"stubs/pep8-naming",
"stubs/psutil",
"stubs/psycopg2",
"stubs/pyflakes",
"stubs/Pygments",
"stubs/PyMySQL",
"stubs/python-dateutil",
"stubs/python-jose",
"stubs/pyvmomi",
"stubs/PyYAML",
"stubs/redis",
"stubs/requests",
"stubs/selenium",
"stubs/Send2Trash",
"stubs/setuptools",
"stubs/simplejson",
"stubs/slumber",
"stubs/SQLAlchemy",
"stubs/stripe",
"stubs/tqdm",
"stubs/tree-sitter",
"stubs/tzlocal/tzlocal/utils.pyi",
"stubs/ttkthemes",
"stubs/urllib3",
"stubs/vobject"
],

Ideally there wouldn't be any stdlib files on that list :)

The main difference between the basic pyright settings in pyrightconfig.json and the stricter pyright settings in pyrightconfig.stricter.json are that with the stricter settings, pyright emits an error if there are any unannotated parameters or return types.

(Note that that doesn't mean that we should go adding Incomplete everywhere if we don't know what the parameter type should be! See #8472 for some recent discussion about why we shouldn't do that. But it does give us something to work towards :)

@AlexWaygood
Copy link
Member

Ideally there wouldn't be any stdlib files on that list :)

(Having said that, I wouldn't bother with trying to fix the distutils or lib2to3 entries. That feels like it would be somewhat of a waste of time, since those modules are deprecated and slated for removal soon :)

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