GeezerVison is showing github.com — register contextvars.Context to collections.abc.Mapping · Issue #126451 · python/cpython
register contextvars.Context to collections.abc.Mapping #126451DescriptionFeature or enhancement
Proposal:
contextvars.Context is documented as implementing the collections.abc.Mapping interface. I believe we should register it to the abc, so an isinstance check can work.
This is similar to #126417 .
Has this already been discussed elsewhere?
This is a minor feature, which does not need previous discussion elsewhere
Links to previous discussion of this feature:
A small amount of prior discussion occurred in an issue for typeshed: python/typeshed#12873
Linked PRs
ActivityJelleZijlstra commented on Nov 5, 2024 Could you list out all the methods on Mapping to confirm it really conforms with the interface? I'd like to prevent another case similar to #125420. tungol commented on Nov 5, 2024 ContributorAuthor More actions Sure.
Mapping methods are: __getitem__, __iter__, __len__, __contains__, keys, items, values, get, __eq__, and __ne__. All are present on Context:
>>> from contextvars import Context
>>> Context.__getitem__
<slot wrapper '__getitem__' of '_contextvars.Context' objects>
>>> Context.__iter__
<slot wrapper '__iter__' of '_contextvars.Context' objects>
>>> Context.__len__
<slot wrapper '__len__' of '_contextvars.Context' objects>
>>> Context.__contains__
<slot wrapper '__contains__' of '_contextvars.Context' objects>
>>> Context.keys
<method 'keys' of '_contextvars.Context' objects>
>>> Context.items
<method 'items' of '_contextvars.Context' objects>
>>> Context.values
<method 'values' of '_contextvars.Context' objects>
>>> Context.get
<method 'get' of '_contextvars.Context' objects>
>>> Context.__eq__
<slot wrapper '__eq__' of '_contextvars.Context' objects>
>>> Context.__ne__
<slot wrapper '__ne__' of '_contextvars.Context' objects>
Reacted by Jelle Zijlstra added a commit that references this issue on Nov 6, 2024gh-126451: Register contextvars.Context to collections.abc.Mapping (#… hauntsaninja commented on Nov 6, 2024 I was surprised that we backported this Reacted by Jelle Zijlstra sobolevn commented on Nov 7, 2024 I was too :)
But, we backported #126419
So, I decided to be consistent. If you think that these backports should be reverted - then I have no objections :) AlexWaygood commented on Nov 7, 2024 The decision to backport is on me -- I applied the backport labels to both PRs. But I also don't feel strongly, and am fine with the backports being reverted if you disagree with them!
FWIW, my thinking was:
- I think this class was always meant to implement a mapping API, and I think it was just an oversight (a micro-bug, IMO) that it was never explicitly registered to the ABC
- I couldn't see any risk of breakage from doing so; it seemed very low-risk
But as I said in #126419 (comment), it feels very borderline to me, so I'm very happy to revert the backports if others disagree! Reacted by sobolevn and Gregory P. Smith JelleZijlstra commented on Nov 7, 2024 The risky aspect to me is that isinstance(..., Mapping) will start behaving differently in a patch release. That may have unexpected effects for some users. Reacted by Gregory P. Smith tungol commented on Nov 7, 2024 ContributorAuthor More actions As the person who kicked this off: I have no opinion about the backports. AlexWaygood commented on Nov 11, 2024 The failing tests in #126674 and #126675 seem to validate folks' concerns about the backports of these PRs. I'll open a revert PR for the backports tomorrow. Reacted by Gregory P. Smith, Jelle Zijlstra, Shantanu and Andrew Svetlov AlexWaygood commented on Nov 11, 2024 Thanks everybody for voicing your concerns! I appreciate it. AlexWaygood commented on Nov 12, 2024 Backports are being reverted in:
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|