-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this 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!).
@@ -383,6 +383,31 @@ def sub(num): | |||
tp.shutdown() | |||
self.assertEqual(results, list(range(10))) | |||
|
|||
def test_token_contextmanager_with_default(self): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 call c.reset()
inside the context manager? or multiple c.set()
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple c.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)
There was a problem hiding this comment.
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.
There was a problem hiding this 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!
There was a problem hiding this 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!
Closes #129889.
📚 Documentation preview 📚: https://cpython-previews--129888.org.readthedocs.build/