Skip to content

Conversation

@willfrey
Copy link
Contributor

It appears to support os.PathLike[str] in addition to just str, based on trying it with pathlib.Path and a custom class implementing __fspath__.

willfrey and others added 2 commits February 10, 2023 12:45
It appears to support `os.PathLike[str]` in addition to just `str`, based on trying it with `pathlib.Path` and a custom class implementing `__fspath__`.
@github-actions

This comment has been minimized.

@willfrey
Copy link
Contributor Author

I also found this, which supports making this change to the annotation

https://github.com/python/cpython/blob/61f2be08661949e2f6dfc94143436297e60d47de/Lib/xml/sax/saxutils.py#L342-L343

@srittau
Copy link
Collaborator

srittau commented Feb 13, 2023

Thanks! I just tested it on Python 3.7 and 3.8 and it seems not to work with 3.7 (but with 3.8). Could you add a version_info check for that?


def parse(
source: str | _SupportsReadClose[bytes] | _SupportsReadClose[str], handler: ContentHandler, errorHandler: ErrorHandler = ...
source: StrPath | _SupportsReadClose[bytes] | _SupportsReadClose[str],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't source also take xmlreader.InputSource ? source is coerced into an InputSource and the docstring also seems to imply one can be passed directly.
https://github.com/python/cpython/blob/3.11/Lib/xml/sax/saxutils.py#L338
https://github.com/python/cpython/blob/3.11/Lib/xml/sax/expatreader.py#L105
https://github.com/python/cpython/blob/3.11/Lib/xml/sax/xmlreader.py#L117

@srittau Dunno if you wanna consider this outside the scope of this PR, because the type for parameter source is missing in a few locations. But I wanted to at least raise this.

@Avasam
Copy link
Collaborator

Avasam commented Feb 13, 2023

Thanks! I just tested it on Python 3.7 and 3.8 and it seems not to work with 3.7 (but with 3.8). Could you add a version_info check for that?

Source to confirm your suspicions: https://github.com/python/cpython/blob/3.7/Lib/xml/sax/saxutils.py#L339

@willfrey
Copy link
Contributor Author

@srittau Can do!

@github-actions
Copy link
Contributor

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

@srittau srittau merged commit 48ef9ea into python:main Feb 13, 2023
@willfrey willfrey deleted the patch-2 branch February 14, 2023 02:13
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