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

Should PackageMetadata.get be supported? #384

Closed
duckinator opened this issue Jun 1, 2022 · 5 comments · Fixed by #444
Closed

Should PackageMetadata.get be supported? #384

duckinator opened this issue Jun 1, 2022 · 5 comments · Fixed by #444

Comments

@duckinator
Copy link

duckinator commented Jun 1, 2022

This isn't a feature request. I'm looking for a quick clarification, so other folks who encounter python/typeshed#7767 have a definitive answer.


In the wild, I've seen things like this:

from importlib.metadata import metadata
print(metadata("bork").get("Version"))

Both the stdlib documentation and the readthedocs documentation never mention .get(), and PackageMetadata does not define it. Due to that, using it and running mypy returns error: "PackageMetadata" has no attribute "get". I suspect I made a bad assumption when I used it, but if so it's one I've seen other folks make — so it seems worthwhile to get an explicit answer on whether leaving it out is deliberate.

So, my question is this:

Is .get() an implementation detail that folks latched on to and should migrate away from? Or should it be added to PackageMetadata?

@rra
Copy link

rra commented Jun 1, 2022

The specific use case I have for using .get() is that sometimes I'm retrieving package metadata that may or may not exist (in support libraries that use package metadata for various purposes, for example). Without .get(), this would require a try/catch block around a hash dereference. That can be awkward, particularly when retrieving multiple bits of metadata at the same time and leaving the ones that aren't set as None.

@jaraco
Copy link
Member

jaraco commented Jun 16, 2022

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

This is another case (similar to #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.

That said, perhaps PackageMetadata should supply .get(). For the standard providers, that method already exists in this implementation. The challenge is that if there are other providers that don't provide .get and the protocol is changed, that implementation won't be in compliance with the protocol. So we'll want a compelling case why .get is valuable.

What about using the .json accessor? It specifies a Dict object, which does have .get. Could that be suitable for your needs?

Or alternately, does PackageMetadata provide enough of Mapping to be able to be cast to a Dict? It seems it does, as it supplies all of the abstract methods of Mapping. That may mean that there's a straightforward way to create an adapter that would provide .get over a PackageMetadata.

@jaraco jaraco changed the title importlib.metadata.metadata(name).get(key) is used in the wild -- is its existence intentional, or an implementation detail? Should PackageMetadata.get be supported? Jun 16, 2022
@duckinator
Copy link
Author

@jaraco thank you for the clarification, I appreciate it! My usecase was resolved by switching from .get(key) to [key], and I was mainly just trying to get clarification for python/typeshed#7767. So my input on .get() itself isn't really relevant. I'll leave the discussion of that to the people it actually affects. :)

@rra
Copy link

rra commented Jun 16, 2022

Supporting get is certainly not strictly necessary, since it could always be replaced with:

try:
    description: Optional[str] = metadata("bork")["Description"]
except KeyError:
    description = None

That's just a little bit irritating.

I suppose:

description = metadata("bork").json.get("Description")

isn't too bad, although sort of weird (and not really documented as the way to get that behavior).

My primary motivation in asking was that it seemed likely that most implementations either already support get or trivially could support get and perhaps not including it in the interface was just an oversight. But if that's not the case, maybe it's not worth it

maxrake added a commit to phylum-dev/phylum-ci that referenced this issue Jul 12, 2022
…in `phylum-init`

* Check for valid versions by using the GitHub API to compare against actual releases
  * Supported target triples programmatically generated based on the results
  * Users will be told when the version provided as an option is not valid
  * Removed `SUPPORTED_TARGET_TRIPLES` static collection
* Add `--list-releases` and `--list-targets` options
  * These are mutually exclusive options
  * Listing Rust target triples are for a specific release version
* Format and refactor throughout
  * Stop using `.get` method on `importlib_metadata.metadata()` results
    * See python/importlib_metadata#384
  * Add constant for `MIN_SUPPORTED_CLI_VERSION`
maxrake added a commit to phylum-dev/phylum-ci that referenced this issue Jul 14, 2022
…in `phylum-init` (#74)

* Check for valid versions by using the GitHub API to compare against actual releases
  * Supported target triples programmatically generated based on the results
  * Users will be told when the version provided as an option is not valid
  * Removed `SUPPORTED_TARGET_TRIPLES` static collection
* Add `--list-releases` and `--list-targets` options
  * These are mutually exclusive options
  * Listing Rust target triples are for a specific release version
* Format and refactor throughout
  * Stop using `.get` method on `importlib_metadata.metadata()` results
    * See python/importlib_metadata#384
  * Add constant for `MIN_SUPPORTED_CLI_VERSION`
@jaraco
Copy link
Member

jaraco commented Apr 7, 2023

Or alternately, does PackageMetadata provide enough of Mapping to be able to be cast to a Dict? It seems it does, as it supplies all of the abstract methods of Mapping. That may mean that there's a straightforward way to create an adapter that would provide .get over a PackageMetadata.

Let me illustrate what I was suggesting here with an example:

description = dict(metadata("bork")).get("Description")

Granted, that approach is a bit convoluted and not performant (creates an entire copy of the metadata to extract one value).

There seems to be a reasonable expectation that PackageMetadata should provide .get, especially after .__getitem__ starts raising KeyError, so let's proceed in that direction and see what happens.

@jaraco jaraco linked a pull request Apr 7, 2023 that will close this issue
jaraco added a commit that referenced this issue Apr 7, 2023
add .get() to the PackageMetadata protocol

Fixes #384.
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 a pull request may close this issue.

3 participants