You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Hi @saaketp. Thanks for the report. I do believe this issue is a bug. I'm yet unsure if the proper fix is to update the protocol to match the implementation or to update the implementation to match the protocol. Historically, the metadata handling relied primarily on the interface of email.message.EmailMessage. That documentation indicates that None is returned for missing values, so maybe the protocol should be updated to match.
Honestly, I'm conflicted. My instinct is that the current protocol definition is right and that __getitem__ should raise a KeyError if a key is missing. I'm unsure why EmailMessage deviated from this typical dict behavior. @warsaw, do you know why EmailMessage does not raise a KeyError for missing headers? Do you have reason to think that importlib.metadata should continue that tradition or should align instead with dict and other typical behavior for __getitem__?
Separately, I'm disappointed that mypy was no help here. As much grief as mypy gives, nitpicking and requiring lots of redundant type hints, it's simultaneously failed here to identify the reported issue, despite the fact that the return type is declared but the returned object is a subclass of a class that violates that declaration. It's conceivable I've missed an important detail that would have allowed mypy to do this validation here.
Looking at the issue that resulted in the change of the return type of email.message.EmailMessage.__getitem__ to Any, looks like it can contain lot more types that just str and Header (python/typeshed#2863) and they chose to return Any instead of object for convenience, but it bites us here.
My vote would be to change the implementation to match the Protocol, I don't like __getitem__ returning None.
Luckily we have a subclass of EmailMessage already, so we can just override the __getitem__ to do an additional check and throw KeyError if the key doesn't exist.
I've just filed bpo-47060python/cpython#91216 (originally python/typeshed#7513) which I think might be dancing around the same root cause. Apologies for intruding; I just wanted to provide a cross-reference to BPO for completeness.
Spurred by considerations in that bpo issue, I'm inclined to say the implementation should be limited to raise a KeyError when an item is not defined in the metadata. I would like to confirm that this library itself does not rely heavily on that expectation.
This backward-incompatible change will likely cause some disruption, so an incremental approach is probably needed to deprecate the existing behavior (warn when None result is returned for a missing key).
Since this change is likely to create disruption, I'd like to reserve it for a new backward-incompatible release to only go into new versions of CPython (3.12 or later), but since it's a refinement of behavior toward the specified behavior, I don't think it needs a deprecation period in CPython's implementation.
I would also like some advice on how code should be structured for cases where one needs to query for metadata that may or may not be there, without triggering the warning.
What's the proper way to write code that can handle a missing item? If you wrap the getitem in a try/except, you still get a deprecation warning.
How about .get(<key>, None)?
Here's a helper you could use:
deffuture_getitem(metadata: PackageMetadata, key: str) ->str:
"""retrieve a key from metadata similar to a future implementation of PackageMetadata.__getitem__"""val=metadata.get(key, None)
ifvalisNone:
raiseKeyError(key)
returnval
Finally, importlib_metadata 8 provides the desired behavior, raising KeyError on a missing key. If you want the KeyError behavior, depend on and use importlib_metadata 8 or later (to be incorporated in Python 3.14).
Activity
[-]Problems with PackageMetadata protocol[/-][+]PackageMetadata.__getitem__ returns None[/+]jaraco commentedon Mar 12, 2022
Hi @saaketp. Thanks for the report. I do believe this issue is a bug. I'm yet unsure if the proper fix is to update the protocol to match the implementation or to update the implementation to match the protocol. Historically, the metadata handling relied primarily on the interface of email.message.EmailMessage. That documentation indicates that
Noneis returned for missing values, so maybe the protocol should be updated to match.jaraco commentedon Mar 12, 2022
Honestly, I'm conflicted. My instinct is that the current protocol definition is right and that
__getitem__should raise aKeyErrorif a key is missing. I'm unsure whyEmailMessagedeviated from this typicaldictbehavior. @warsaw, do you know whyEmailMessagedoes not raise aKeyErrorfor missing headers? Do you have reason to think thatimportlib.metadatashould continue that tradition or should align instead with dict and other typical behavior for__getitem__?jaraco commentedon Mar 12, 2022
Separately, I'm disappointed that
mypywas no help here. As much grief as mypy gives, nitpicking and requiring lots of redundant type hints, it's simultaneously failed here to identify the reported issue, despite the fact that the return type is declared but the returned object is a subclass of a class that violates that declaration. It's conceivable I've missed an important detail that would have allowed mypy to do this validation here.Perhaps the issue is that typeshed underdefines the return type as Any. Perhaps someone would like to fix that.
saaketp commentedon Mar 12, 2022
Looking at the issue that resulted in the change of the return type of
email.message.EmailMessage.__getitem__to Any, looks like it can contain lot more types that just str and Header (python/typeshed#2863) and they chose to returnAnyinstead ofobjectfor convenience, but it bites us here.My vote would be to change the implementation to match the Protocol, I don't like
__getitem__returningNone.Luckily we have a subclass of
EmailMessagealready, so we can just override the__getitem__to do an additional check and throwKeyErrorif the key doesn't exist.DMRobertson commentedon Mar 18, 2022
I've just filed
bpo-47060python/cpython#91216 (originally python/typeshed#7513) which I think might be dancing around the same root cause. Apologies for intruding; I just wanted to provide a cross-reference to BPO for completeness.jaraco commentedon Mar 19, 2022
Spurred by considerations in that bpo issue, I'm inclined to say the implementation should be limited to raise a KeyError when an item is not defined in the metadata. I would like to confirm that this library itself does not rely heavily on that expectation.
This backward-incompatible change will likely cause some disruption, so an incremental approach is probably needed to deprecate the existing behavior (warn when
Noneresult is returned for a missing key).jaraco commentedon Mar 19, 2022
In 614a522, I confirmed that at least internally, there's no issue with raising KeyError on missing keys.
jaraco commentedon May 21, 2022
Since this change is likely to create disruption, I'd like to reserve it for a new backward-incompatible release to only go into new versions of CPython (3.12 or later), but since it's a refinement of behavior toward the specified behavior, I don't think it needs a deprecation period in CPython's implementation.
Add test capturing that a missing key raises a KeyError. Ref #371.
When requesting an item from metadata, if it's not present, raise a K…
jaraco commentedon Dec 18, 2022
Before the release could even hit PyPI, the backward incompatibility struck. The release process failed because
twineis relying on__getitem__returningNone.This finding leads me to believe there may need to be a deprecation period for this change.
Add test capturing that a missing key raises a KeyError. Ref #371.
When requesting an item from metadata, if it's not present, raise a K…
Mark `PackageMetadata.__getitem__` as deprecated for missing values. …
Skip distributions with incomplete metadata
Skip distributions with incomplete metadata
Skip distributions with incomplete metadata (#2231)
encukou commentedon Oct 3, 2023
What's the proper way to write code that can handle a missing item?
If you wrap the getitem in a
try/except, you still get a deprecation warning.cc @hroncok
int19h commentedon Feb 23, 2024
I would also like some advice on how code should be structured for cases where one needs to query for metadata that may or may not be there, without triggering the warning.
jaraco commentedon Mar 20, 2024
How about
.get(<key>, None)?Here's a helper you could use:
Should importlib_metadata provide this helper?
Message.__getitem__ now raises a KeyError on missing keys.
jaraco commentedon Jun 23, 2024
Finally, importlib_metadata 8 provides the desired behavior, raising KeyError on a missing key. If you want the KeyError behavior, depend on and use importlib_metadata 8 or later (to be incorporated in Python 3.14).