Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-129889: Support context manager protocol by contextvars.Token#129888

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
asvetlov merged 13 commits intopython:mainfromasvetlov:cm_token
Feb 12, 2025

Conversation

@asvetlov
Copy link
Contributor

@asvetlovasvetlov commentedFeb 9, 2025
edited
Loading

@asvetlovasvetlov changed the titleSupport context manager protocol by contextvars.Tokengh-129889: Support context manager protocol by contextvars.TokenFeb 9, 2025
Copy link
Member

@picnixzpicnixz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Since it's a new feature, we also need a What's New entry as well (I think you already know it and since it's a draft, you might be writing it at the moment, but it's just a friendly reminder!).

tp.shutdown()
self.assertEqual(results,list(range(10)))

deftest_token_contextmanager_with_default(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Maybe more test with a re-entrant context? as well as a test when an exception is raised in the context body?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks for the review, I'll add tests.
I don't expect any problems though since the implementation is really trivial.

Copy link
Member

@picnixzpicnixzFeb 9, 2025
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Up to you though! Generally, I'm not actually adding those kind of tests to test the implementation but rather to spot possible regressions if we change something. I actually don't know if we are that pedantic when testing other context managers so feel free not to burden the tests if you think it's not worth it!

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Tests are cheap and easy to write,
I've added two: one for exit when an exception is raised and another for reentrancy.

Please feel free to ask for additional tests if needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks good! Small question, but is there a necessity to check what happens if we callc.reset() inside the context manager? or multiplec.set() as well?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Multiplec.set() doesn't affect the context manager, the value will be reset on exit.
c.reset() is safe if the other token was used; resetting with the same token raises RuntimeErorr as in the following example:

with c.set(1) as token:    c.reset(token)

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

A couple additional tests were added to demonstrate mentioned scenarios.

picnixz reacted with heart emoji
Copy link
Member

@picnixzpicnixz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks for those tests! I think they are worth it since they capture non-trivial cases even if the implementation is trivial. Except for the changelog entries to be written I think we got a pretty good coverage!

@asvetlovasvetlov marked this pull request as ready for reviewFebruary 10, 2025 08:50
@asvetlov
Copy link
ContributorAuthor

@picnixz news is added, and the PR is ready for review.

@1st1, maybe you can find time for the review as the author of contextvars?

picnixz reacted with hooray emoji

Copy link
Member

@picnixzpicnixz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks good to me!

@asvetlovasvetlov merged commit469d2e4 intopython:mainFeb 12, 2025
42 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@picnixzpicnixzpicnixz approved these changes

@1st11st1Awaiting requested review from 1st11st1 is a code owner

Assignees

@asvetlovasvetlov

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support context manager protocol by contextvars.Token

2 participants

@asvetlov@picnixz

[8]ページ先頭

©2009-2025 Movatter.jp