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

pkg_resources: Expose the type of imports and re-exports #4242

Closed

Conversation

Avasam
Copy link
Contributor

@Avasam Avasam commented Feb 27, 2024

Summary of changes

Works towards #2345 and a big step towards merging typeshed's pkg_resources stubs into setuptools
doesn't loose exposed type information from typeshed.

Pull Request Checklist

  • Changes have tests (this is to help with type-checking checks)
  • News fragment added in newsfragments/.
    (See documentation for details)

pkg_resources/__init__.py Outdated Show resolved Hide resolved
pkg_resources/__init__.py Outdated Show resolved Hide resolved
import packaging.specifiers
import packaging.requirements
import packaging.markers
import packaging.utils
Copy link
Contributor

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 in pkg_resources/__init__.py or pkg_resouces/extern/__init__.py, probably the comments apply to both).

At the end of the day, mypy cannot handle MetaPathFinders... 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 a mypy 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

  1. 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.

Copy link
Contributor Author

@Avasam Avasam Mar 5, 2024

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 subclassing Any for RequirementParseError and Requirement

With static imports:
image

W/o the import behind a TYPE_CHECKING:
image


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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor

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 MetaPathFinders in type checkers to Jason.

Copy link
Contributor Author

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.

pkg_resources/__init__.py Outdated Show resolved Hide resolved
@Avasam Avasam changed the title pkg_resources: Fix type of pre-declared variables & imports + remove unused pkg_resources: Fix type of pre-declared variables & imports Mar 6, 2024
@Avasam Avasam changed the title pkg_resources: Fix type of pre-declared variables & imports pkg_resources: Expose the type of imports and re-exports Mar 8, 2024
@Avasam
Copy link
Contributor Author

Avasam commented May 2, 2024

Setting as draft to make it clear this isn't a priority to review. Will probably end up being the last pkg_resources PR for static typing.

@Avasam Avasam marked this pull request as draft May 2, 2024 17:05
@Avasam
Copy link
Contributor Author

Avasam commented Jul 17, 2024

Superseded by #4457 , thanks @jaraco !

Static typing issues caused by vendored packages can now also be fixed upstream

@Avasam Avasam closed this Jul 17, 2024
@Avasam Avasam deleted the pkg_resources-Fix-type-of-pre-declared-variables branch July 17, 2024 21:14
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