Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
pkg_resources
: Expose the type of imports and re-exports #4242pkg_resources
: Expose the type of imports and re-exports #4242Changes from 2 commits
1827bfb
fbcca3b
76be263
e0da6b2
cebb410
e19f4a9
255c283
a9637b4
b68edb4
7df3648
594900a
9db33f4
5b19c27
214168e
71e624b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit tricky...
Based on the comments:
I suppose this would be a controversial change (I am not sure if it matters to have the
if TYPE_CHECKING
inpkg_resources/__init__.py
orpkg_resouces/extern/__init__.py
, probably the comments apply to both).At the end of the day,
mypy
cannot handleMetaPathFinder
s... I suppose that the ultimate solution would be to use a mypy plugin to "convince"mypy
to use the files in the*/_vendor
folder for the*.extern
modules, but that sounds very complicated1. So there is a trade-off here: complexity of implementing amypy
plugin vs. in the duplication in*.extern
modules vs. leave the things unchecked (and loosing some of the benefits of the type analysis).@jaraco, do you have any opinion about this one?
Footnotes
I cannot find a hook for finding modules in the docs: https://mypy.readthedocs.io/en/stable/extending_mypy.html#extending-mypy-using-plugins. So I opened https://github.com/python/mypy/issues/16988 for guidance on this. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's slightly different from what I originally did in #3979 in that it's only duplicating what's needed/used. And doing so right next to the equivalent dynamic imports. So imo there's less of a maintenance concern.
packaging.requirements
is especially important to avoid looking like we're subclassingAny
forRequirementParseError
andRequirement
With static imports:
W/o the import behind a
TYPE_CHECKING
:FWIW, I'd rather loose on type safety internally, publicly loose on implicit return types (until all public API return types are explicit), and loose on some super class typing, than going for a mypy plugin. Which also only works on mypy, not pyright, VSCode+Pylance, or PyCharm.
I'll probably end up keeping those changes in my local working branches since it helps me validate the types and understand intended behaviour. Even if it doesn't end up in
main
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hang on, now that there's a
cog
script/template thingy, could I extend it to do #3979 (comment) again but automated this time?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't see why not. But currently there is a problem with the script that I am fixing in #4353, so it might make sense to wait for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, I will defer the final decision about any workarounds for the lack of support of
MetaPathFinder
s in type checkers to Jason.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'm likely gonna do this last anyway. It's just good to know I have more options to try and suggest.