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

importlib.metadata.PackageMetadata has no get method? #7767

Closed
rra opened this issue May 2, 2022 · 8 comments
Closed

importlib.metadata.PackageMetadata has no get method? #7767

rra opened this issue May 2, 2022 · 8 comments
Labels
status: deferred Issue or PR deferred until some precondition is fixed

Comments

@rra
Copy link
Contributor

rra commented May 2, 2022

Since the return value for importlib.metadata.metadata was fixed in #7331, mypy now incorrectly diagnoses calls to get as a call to an invalid method. That PR took the approach of importing PackageMetadata from the relevant module, but that protocol does not apparently capture the entire supported interface.

(Maybe this is a bug in my understanding of the return value of metadata and get isn't a valid call? But I have a lot of code that does this and it's always worked.)

% cat test.py
from importlib.metadata import metadata
print(metadata("mypy").get("Version"))
% python test.py
0.950
% mypy test.py
test.py:2: error: "PackageMetadata" has no attribute "get"
Found 1 error in 1 file (checked 1 source file)
rra added a commit to lsst/templates that referenced this issue May 2, 2022
The current mypy plus typeshed plus the Python 3.10 core library
has a bug causing mypy to not think get is a valid method on the
return value of importlib.metadata.metadata because it returns a
Protocol that doesn't implement get.  I've filed a issue in case
this is not intended (python/typeshed#7767),
but we know that any package created by this template will always
have Summary and Version metadata, so we can use them
unconditionally.

Previously, this may not have worked in situations where the
package was being used uninstalled, but since we now do everything
including local development servers from inside tox, and make init
also installs the package for running pytest directly, this should
no longer be an issue.  setuptools_scm should always generate a
version, even if it's an unuseful one.
rra added a commit to lsst-sqre/safir that referenced this issue May 2, 2022
In the latest mypy (0.942), importlib.metadata.metadata now returns
a Protocol named PackageMetadata that doesn't implement the get
method.  The actual object remains a Message class, however, so the
get method works correctly.  Only mypy checking fails.

Pending python/typeshed#7767, which may
reveal that it is intentional that get not be supported, cast the
return value to Message so that the existing functions work.  This
seems better than adding try/catch blocks for every piece of
metadata of interest given that the omission of get appears to be
unintentional.
@duckinator
Copy link

duckinator commented May 31, 2022

I ran into this problem too. get() isn't documented in the importlib.metadata docs ( https://docs.python.org/3/library/importlib.metadata.html ), so mypy/typeshed's behavior is matching the documentation. So I guess the real question would be, is importlib.metadata(pkg).get(key) part of the defined API, or an implementation detail? It looks like it might the latter, but that would surprise me a lot.

@AlexWaygood
Copy link
Member

AlexWaygood commented Jun 1, 2022

I ran into this problem too. get() isn't documented in the importlib.metadata docs ( https://docs.python.org/3/library/importlib.metadata.html ), so mypy/typeshed's behavior is matching the documentation. So I guess the real question would be, is importlib.metadata(pkg).get(key) part of the defined API, or an implementation detail? It looks like it might the latter, but that would surprise me a lot.

I don't think this is a question that we, at typeshed, can really answer. We're basically taking the type hints from upstream here, and I think — since importlib.metadata is one of the few stdlib modules that is typed — that that's probably the right thing to do. So if I were you, I'd probably ask for clarification over at the https://github.com/python/importlib_metadata issue tracker about why the PackageMetadata protocol doesn't have a .get() method.

@AlexWaygood AlexWaygood changed the title mypy incorrectly diagnoses invalid get method for PackageMetadata importlib.metadata.PackageMetadata has no get method? Jun 1, 2022
@duckinator
Copy link

duckinator commented Jun 1, 2022

I don't think this is a question that we, at typeshed, can really answer.

Yeah, sorry for the weird phrasing there — I meant that the question is more "is there supposed to be a .get() method?" as opposed to "why did typeshed leave out the .get() method?"

As you pointed out, that question is better answered by the importlib_metadata folks.

I'll open an issue over there asking about it. 🙂

@duckinator
Copy link

Opened python/importlib_metadata#384 asking for a clarification on whether it's an implementation detail that should be migrated away from.

@AlexWaygood
Copy link
Member

Opened python/importlib_metadata#384 asking for a clarification on whether it's an implementation detail that should be migrated away from.

Thank you!

@AlexWaygood
Copy link
Member

I'm marking this issue as "deferred" until we get an answer from upstream

@AlexWaygood AlexWaygood added the status: deferred Issue or PR deferred until some precondition is fixed label Jun 12, 2022
@duckinator
Copy link

There's some more details in python/importlib_metadata#384 (comment) , but I'll quote the part that seems directly-relevant here:

tl;dr - yes, it's an implementation detail.

This is another case (similar to python/importlib_metadata#371), where the duck-typing approach has left ambiguity. This project has provided a viable implementation but not a rigorously specified one. In more recent releases, PackageMetadata was introduced to be explicit about the supported interface. That answers the question in the title. It's an implementation detail that was not expected to be used.

The rest of the comment mentions possibly adding it, but right now typeshed is correct.

@AlexWaygood
Copy link
Member

The rest of the comment mentions possibly adding it, but right now typeshed is correct.

I don't think there's anything actionable for typeshed here unless and until the runtime decides that .get() should be added to the protocol at runtime. So I'll close this for now — but feel free to open a new issue if the situation changes :)

@AlexWaygood AlexWaygood closed this as not planned Won't fix, can't repro, duplicate, stale Jun 20, 2022
s-weigand added a commit to s-weigand/verbose-version-info that referenced this issue Aug 27, 2022
s-weigand added a commit to s-weigand/verbose-version-info that referenced this issue Aug 27, 2022
* ⬆️ UPGRADE: Autoupdate pre-commit config

* 🔧🧹 Removed interrogate pre-push hook

* 🩹📚 Fixed typo

* 🔧👌 Sorted pre-commit hooks to run more effectively

* 🔧🩹 Fixed rstcheck hook config

* 🔧🧹 Ignore false positive type error

Ref.: python/typeshed#7767

Co-authored-by: s-weigand <s-weigand@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: deferred Issue or PR deferred until some precondition is fixed
Projects
None yet
Development

No branches or pull requests

3 participants