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

register contextvars.Context to collections.abc.Mapping #126451

Closed
tungol opened this issue Nov 5, 2024 · 10 comments
Closed

register contextvars.Context to collections.abc.Mapping #126451

tungol opened this issue Nov 5, 2024 · 10 comments
Assignees
Labels
stdlib Python modules in the Lib dir topic-typing type-feature A feature request or enhancement

Comments

@tungol
Copy link
Contributor

tungol commented Nov 5, 2024

Feature 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

@tungol tungol added the type-feature A feature request or enhancement label Nov 5, 2024
@JelleZijlstra
Copy link
Member

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
Copy link
Contributor Author

tungol commented Nov 5, 2024

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>

@picnixz picnixz added topic-typing stdlib Python modules in the Lib dir labels Nov 6, 2024
sobolevn added a commit that referenced this issue Nov 6, 2024
…126452)

Co-authored-by: sobolevn <mail@sobolevn.me>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Nov 6, 2024
…ing (pythonGH-126452)

(cherry picked from commit 5dc36dc)

Co-authored-by: Stephen Morton <git@tungol.org>
Co-authored-by: sobolevn <mail@sobolevn.me>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Nov 6, 2024
…ing (pythonGH-126452)

(cherry picked from commit 5dc36dc)

Co-authored-by: Stephen Morton <git@tungol.org>
Co-authored-by: sobolevn <mail@sobolevn.me>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
sobolevn added a commit that referenced this issue Nov 6, 2024
…ping (GH-126452) (#126519)

gh-126451: Register contextvars.Context to collections.abc.Mapping (GH-126452)
(cherry picked from commit 5dc36dc)

Co-authored-by: Stephen Morton <git@tungol.org>
Co-authored-by: sobolevn <mail@sobolevn.me>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
sobolevn added a commit that referenced this issue Nov 6, 2024
…ping (GH-126452) (#126518)

gh-126451: Register contextvars.Context to collections.abc.Mapping (GH-126452)
(cherry picked from commit 5dc36dc)

Co-authored-by: Stephen Morton <git@tungol.org>
Co-authored-by: sobolevn <mail@sobolevn.me>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
@tungol tungol closed this as completed Nov 6, 2024
@hauntsaninja
Copy link
Contributor

I was surprised that we backported this

@sobolevn
Copy link
Member

sobolevn commented 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
Copy link
Member

AlexWaygood commented 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!

@JelleZijlstra
Copy link
Member

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.

@tungol
Copy link
Contributor Author

tungol commented Nov 7, 2024

As the person who kicked this off: I have no opinion about the backports.

@AlexWaygood
Copy link
Member

AlexWaygood commented 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.

@AlexWaygood AlexWaygood reopened this Nov 11, 2024
@AlexWaygood AlexWaygood self-assigned this Nov 11, 2024
@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 11, 2024

Thanks everybody for voicing your concerns! I appreciate it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir topic-typing type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

6 participants