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

FileFinder takes a class, not an instance (#7085) #7086

Merged
merged 1 commit into from
Jan 30, 2022
Merged

FileFinder takes a class, not an instance (#7085) #7086

merged 1 commit into from
Jan 30, 2022

Conversation

Spindel
Copy link
Contributor

@Spindel Spindel commented Jan 30, 2022

The FileFinder takes a tuple of (class, arguments) to instantiate for
each time, rather than an existing instance.

This fixes the type hint to match Python 3.10 reality.

Issue: #7085

@github-actions

This comment has been minimized.

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.

from __future__ import annotations is implicitly enabled in stub files, so we can just use builtin type instead of typing.Type — this is what the flake8 error message was trying to flag.

(Do you have any feedback for how we could improve that flake8 error message, btw? 🙂)

stdlib/importlib/machinery.pyi Outdated Show resolved Hide resolved
stdlib/importlib/machinery.pyi Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

I have opened PyCQA/flake8-pyi#164 — we can definitely improve that error message :)

As the person who, er, wrote the error message, I sincerely apologise for the confusion!!

@github-actions

This comment has been minimized.

The FileFinder takes a tuple of (class, arguments) to instantiate for
each time, rather than an existing instance.

This fixes the type hint to match Python 3.10 reality.

Issue: #7085
@Spindel
Copy link
Contributor Author

Spindel commented Jan 30, 2022

(Do you have any feedback for how we could improve that flake8 error message, btw? slightly_smiling_face)

Perhaps Y022 It is preferred to use type[C] over typing.Type[C]

@Spindel
Copy link
Contributor Author

Spindel commented Jan 30, 2022

Anyhow, I've squashed away the evidence of my confusion, and it should pass the lints now, thanks for your help!

@AlexWaygood
Copy link
Member

Anyhow, I've squashed away the evidence of my confusion, and it should pass the lints now, thanks for your help!

No problem!!

@github-actions
Copy link
Contributor

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

@Spindel
Copy link
Contributor Author

Spindel commented Jan 30, 2022

As for how I ended up here, due to some arguments and a slight insomnia I ended up writing a Python module to enforce type hints on all imports at run-time and not as a CI step.

It turns out it ended up shooting itself as the importlib types didn't match reality <.<

But once that was fixed, it actually works, with only slight side-effects (may include nausea, sluggishness and ImportErrors.)

@JelleZijlstra JelleZijlstra merged commit 1365926 into python:master Jan 30, 2022
@JelleZijlstra
Copy link
Member

Thank you! (And thanks Alex for improving the message)

@Spindel Spindel deleted the stdlib-importlib-machinery-loader branch January 30, 2022 16:47
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.

None yet

3 participants