Skip to content

Add stubs for importlib.(resources.)readers #10928

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

Merged
merged 6 commits into from
Oct 26, 2023

Conversation

AlexWaygood
Copy link
Member

The annotations are incomplete. The module is somewhat poorly documented, and I wanted to defer anything too complex to future PRs, so that this was easier to review

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member Author

I'm not sure why 8232196 was needed. I thought both mypy_test.py and pyright had special handling for our stdlib/VERSIONS file, so that we didn't need to do things like this, but both were failing without that change. It's also confusing to me that stubtest passed, even though mypy_test.py didn't.

@AlexWaygood AlexWaygood marked this pull request as ready for review October 26, 2023 12:03
def __init__(self, loader, module: str) -> None: ...
def open_resource(self, resource: str) -> BufferedReader: ...
def is_resource(self, path: StrPath) -> bool: ...
def files(self): ...
Copy link
Member Author

Choose a reason for hiding this comment

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

This obviously returns a zipfile.Path at runtime. Unfortunately, if you add that annotation right now, mypy complains:

stdlib\importlib\readers.pyi:39: error: Return type "Path" of "files" incompatible with return type "Traversable" in supertype "TraversableResources"  [override]

This points to an error either in our zipfile stubs or in our importlib.(resources.)abc stubs. Type checkers should probably understand zipfile.Path as being a subtype of Traversable; I believe that's the intention of the authors of these modules.

Anyway, that looks non-trivial to fix, so I'd rather defer it to another PR for now.

@github-actions

This comment has been minimized.

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! A few things I noticed below.

else:
def joinpath(self, child: str) -> abc.Traversable: ... # type: ignore[override]
__truediv__ = joinpath
def open(self, *args: Never, **kwargs: Never) -> NoReturn: ... # type: ignore[override]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def open(self, *args: Never, **kwargs: Never) -> NoReturn: ... # type: ignore[override]
def open(self, *args: Unused, **kwargs: Unused) -> NoReturn: ... # type: ignore[override]

Never would disallow arguably allowed calls like .open("r") (even if those always raise an exception, of course).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that was a deliberate choice... I thought maybe it would be useful for type checkers to warn you if you provided arguments to this function. Since it always fails at runtime, it seems like you're probably making a mistake if you're passing arguments to it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

But that should already be caught by the NoReturn.While this is a far-fetched example (using a non-existent typing feature), this disallows things like this:

trav_open: SignatureOf[Traversible.open] = MultiplexedPath.open

I wonder why no type checker complains about using Never, since this breaks Traversible's contract.

That said, I don't feel strongly about this.

Copy link
Member Author

@AlexWaygood AlexWaygood Oct 26, 2023

Choose a reason for hiding this comment

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

But that should already be caught by the NoReturn

If we only have the NoReturn return annotation, type checkers will only complain when you call this method if you have mypy's --warn-unreachable setting turned on (or the equivalent setting in other type checkers), and most people don't, unfortunately. Whereas annotating the arguments with Never means you'll get an error from the type checker even if you only have the default settings enabled.

I'll concede that this is a bit of a hack, and that we're really working around the lack of something like python/typing#1043 -- but I think it's useful for the time being

@github-actions
Copy link
Contributor

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

pytest (https://github.com/pytest-dev/pytest)
+ src/_pytest/assertion/rewrite.py:295: error: Unused "type: ignore" comment  [unused-ignore]

@AlexWaygood AlexWaygood merged commit 908993a into python:main Oct 26, 2023
@AlexWaygood AlexWaygood deleted the importlib-resources branch October 26, 2023 13:18
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