Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
bpo-30241: implement contextlib.AbstractAsyncContextManager#1412
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
JelleZijlstra commented May 3, 2017 • edited by bedevere-bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-bot
Uh oh!
There was an error while loading. Please reload this page.
mention-bot commented May 3, 2017
@JelleZijlstra, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ncoghlan, @birkenfeld and @rhettinger to be potential reviewers. |
Lib/contextlib.py Outdated
| def __subclasshook__(cls, C): | ||
| if cls is AbstractAsyncContextManager: | ||
| if (any("__aenter__" in B.__dict__ for B in C.__mro__) and | ||
| any("__aexit__" in B.__dict__ for B in C.__mro__)): |
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 understand you just copied this from above, but I think both should be updated to support the explicit "anti-registration" __enter__ = None pattern, like all classes in collections.abc, see _check_methods function in _collections_abc.py.
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 that should be a separate issue? AbstractContextManager should arguably be fixed even in 3.6, and this PR only targets 3.7. I can fix AbstractAsyncContextManager though.
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.
Yes, a separate PR for 3.6 is needed. But I think it makes sense to already fix the async version here.
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.
Filed http://bugs.python.org/issue30266 to track that.
gvanrossum commented Jun 9, 2017
Wait, isn't this a new feature that should go into 3.7? |
ilevkivskyi commented Jun 9, 2017
Oh yes, sorry, this one is not urgent then. |
Implements: - python/typing#438 - python/cpython#360python/cpython#1412, which adds contextlib.AbstractAsyncContextManager, has not yet been merged.
) Implements: - python/typing#438 - python/cpython#360 Note that python/cpython#1412, which adds contextlib.AbstractAsyncContextManager, has not yet been merged.
1st1 commented Oct 10, 2017
Please add a NEWS entry and I'll merge this in. |
JelleZijlstra commented Oct 11, 2017
Thanks, just pushed a NEWS entry. |
Doc/library/contextlib.rst Outdated
| .. class:: AbstractAsyncContextManager | ||
| An :term:`abstract base class` similar to |
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'm wondering if we could just copy the paragraph from AbstractContextManager doc. "similar to" isn't easy to read here.
Kentzo commented Dec 11, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
The PR #4790 adds |
ilevkivskyi commented Dec 13, 2017
@JelleZijlstra Could you please fix the merge conflict and update the docs as @1st1 proposed? I would like to have this merged soon. |
1st1 commented Dec 14, 2017
Thanks @JelleZijlstra! |
JelleZijlstra commented Dec 14, 2017
Thanks, and sorry for dropping the ball on making the docs change! |
1st1 commented Dec 14, 2017
NP ;) |
ilevkivskyi commented Dec 14, 2017
Thank you @JelleZijlstra and @1st1 ! |
https://bugs.python.org/issue30241