-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
bpo-29679: Implement @contextlib.asynccontextmanager #360
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
Changes from 1 commit
904e8a8
b3d59f1
a1d5b3f
c5b8b43
ca77cd2
689f4a5
299d968
e974d48
5808a4c
9caa243
64e6908
178433b
6d0dddb
ad65b4d
737fd0f
3fc20a7
06697a8
bb8de0d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
Needs docs and tests
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,8 +54,9 @@ def inner(*args, **kwds): | |
return inner | ||
|
||
|
||
class _GeneratorContextManager(ContextDecorator, AbstractContextManager): | ||
"""Helper for @contextmanager decorator.""" | ||
class _GeneratorContextManagerBase(ContextDecorator): | ||
"""Shared functionality for the @contextmanager and @asynccontextmanager | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. First line of docstring must be a single sentence. Just make it """Shared functionality for the @contextmanager and @asynccontextmanager.""" |
||
implementations.""" | ||
|
||
def __init__(self, func, args, kwds): | ||
self.gen = func(*args, **kwds) | ||
|
@@ -77,6 +78,10 @@ def _recreate_cm(self): | |
# called | ||
return self.__class__(self.func, self.args, self.kwds) | ||
|
||
|
||
class _GeneratorContextManager(_GeneratorContextManagerBase, AbstractContextManager): | ||
"""Helper for @contextmanager decorator.""" | ||
|
||
def __enter__(self): | ||
try: | ||
return next(self.gen) | ||
|
@@ -126,6 +131,44 @@ def __exit__(self, type, value, traceback): | |
raise | ||
|
||
|
||
class _AsyncGeneratorContextManager(_GeneratorContextManagerBase): | ||
"""Helper for @asynccontextmanager.""" | ||
|
||
async def __aenter__(self): | ||
try: | ||
return await self.gen.__anext__() | ||
except StopAsyncIteration: | ||
raise RuntimeError("generator didn't yield") from None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Diff coverage shows a missing test case for this line. |
||
|
||
async def __aexit__(self, type, value, traceback): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
if type is None: | ||
try: | ||
await self.gen.__anext__() | ||
except StopAsyncIteration: | ||
return | ||
else: | ||
raise RuntimeError("generator didn't stop") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing test case here as well. |
||
else: | ||
if value is None: | ||
value = type() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You won't be able to hit this line via the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could also write a small C helper to do that, but your idea is much better. |
||
# See _GeneratorContextManager.__exit__ for comments on subtleties | ||
# in this implementation | ||
try: | ||
await self.gen.athrow(type, value, traceback) | ||
raise RuntimeError("generator didn't stop after throw()") | ||
except StopAsyncIteration as exc: | ||
return exc is not value | ||
except RuntimeError as exc: | ||
if exc is value: | ||
return False | ||
if exc.__cause__ is value: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huh, this (and its counterpart in Don't fix the synchronous version in this PR, but do add a test case for the async version that hits this line (throw |
||
return False | ||
raise | ||
except: | ||
if sys.exc_info()[1] is not value: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why we use except BaseException as exc:
if exc is not value:
raise There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did this in order to not make unnecessary changes relative to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It uses sys.exc_info() because the corresponding _GeneratorContextManager code predates the acceptance and implementation of PEP 352, and is also common between 2.7 and 3.x (and hence needs to handle exceptions that don't inherit from I'm fine with switching this to using the |
||
raise | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hitting this line means adding a test case that replaces the thrown in exception with an entirely unrelated one that is neither |
||
|
||
|
||
def contextmanager(func): | ||
"""@contextmanager decorator. | ||
|
||
|
@@ -160,6 +203,40 @@ def helper(*args, **kwds): | |
return helper | ||
|
||
|
||
def asynccontextmanager(func): | ||
"""@contextmanager decorator. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing |
||
|
||
Typical usage: | ||
|
||
@asynccontextmanager | ||
async def some_async_generator(<arguments>): | ||
<setup> | ||
try: | ||
yield <value> | ||
finally: | ||
<cleanup> | ||
|
||
This makes this: | ||
|
||
async with some_async_generator(<arguments>) as <variable>: | ||
<body> | ||
|
||
equivalent to this: | ||
|
||
<setup> | ||
try: | ||
<variable> = <value> | ||
<body> | ||
finally: | ||
<cleanup> | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Trailing empty newline. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean I should remove or add a newline? The formatting here is the same as in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd remove both empty newlines then. They aren't needed. |
||
""" | ||
@wraps(func) | ||
def helper(*args, **kwds): | ||
return _AsyncGeneratorContextManager(func, args, kwds) | ||
return helper | ||
|
||
|
||
class closing(AbstractContextManager): | ||
"""Context to automatically close something at the end of a block. | ||
|
||
|
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.
ContextDecorator is designed to work with
__call__
rather than__await__
, so having it also applied toasynccontextmanager
doesn't look right to me.So I'd suggest moving the ContextDecorator inheritance and the
_recreate_cm
method down into_GeneratorContextManager
and only keeping the__init__
method common between the two.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.
You're right, my implementation didn't actually work as a decorator. (Although it's because ContextDecorator uses
with
instead ofasync with
, not because of anything to do with__call__
.)We could add a separate
AsyncContextDecorator
implementation, but I don't currently have a good use for that.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.
I actually consider making _GeneratorContextManager inherit from ContextDecorator a design error on my part (that's why _recreate_cm is still a private API: https://bugs.python.org/issue11647#msg161113 )
So simply not supporting an equivalent for async context managers is entirely fine by me :)