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

Adopt "more sound" type annotations #487

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

abravalheri
Copy link
Contributor

@abravalheri abravalheri commented May 2, 2024

Motivated by #486.

This PR just provides more realistic type annotations1 to improve the type checking soundness of the package.

It does not solve #486: all the limitations are still present, but at least type checkers would help to notify users when they are trying to use new APIs that are not safe (not necessary all the time, but better than now).

It is a bit of a pain, but these annotations should be a bit more sound than they currently are.

This should also prevent future versions of this library from introducing incompatible behaviour (thanks to pytest-mypy).

Footnotes

  1. Technically speaking, "a bit more correct". Because importlib_metadata collects 3rd-party implementations via find_distributions everything can happen.

@abravalheri abravalheri changed the title Adopt more realistic type annotations Adopt "more sound" type annotations May 2, 2024
@abravalheri abravalheri marked this pull request as ready for review May 2, 2024 10:44
@jaraco
Copy link
Member

jaraco commented Jun 23, 2024

Thanks for the proposal. I'm going to have to think about this more.

This library is merged back into CPython as importlib.metadata so these annotations won't make sense when that happens. Moreover, this is the first introduction of references to importlib.metadata in this library, so I'm unsure what the implications will be.

Is there any way to achieve the objectives either by:

  • adjusting the patch to be universal (so it's meaningful in both importilb_metadata and importlib.metadata), or
  • moving the backport-specific concerns to a ./compat/* module that would be omitted downstream in CPython?

@abravalheri
Copy link
Contributor Author

Thanks @jaraco for the code review.

Is there any way to achieve the objectives either by:

  • adjusting the patch to be universal (so it's meaningful in both importilb_metadata and importlib.metadata), or

Unfortunately I don't think so.
importlib_metadata has a backwards compatibility problem that simply disappears when it becomes importlib.metadata.

In this sense only importlib_metadata need these contrived type annotations to inform end-users that they can get back legacy objects when using the API.

  • moving the backport-specific concerns to a ./compat/* module that would be omitted downstream in CPython?

I gave it a try... don't know if that is exactly what you had in mind, but I tried to do my best.
Please note that the annotations are not specific to a version of Python, but always will be necessary while importlib_metadata exists in PyPI. In the future I believe they may be useful during development (e.g. by identifying places where legacy objects are retrieved and preventing incompatible methods to be called on them).

I am not good with names, so if you have suggestions regarding names please let me know.

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