FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-30241: implement contextlib.AbstractAsyncContextManager by JelleZijlstra · Pull Request #1412 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

bpo-30241: implement contextlib.AbstractAsyncContextManager - #1412

Merged
1st1 merged 6 commits into
python:masterfrom
JelleZijlstra:aacm
Dec 14, 2017
ConversationCommits (6)ChecksFiles changed
Merged

bpo-30241: implement contextlib.AbstractAsyncContextManager#1412
1st1 merged 6 commits into
python:masterfrom
JelleZijlstra:aacm

Conversation

JelleZijlstra commented May 3, 2017 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

mention-bot commented May 3, 2017

Copy link
Copy Markdown

@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.

ilevkivskyi reviewed May 3, 2017
View reviewed changes
Comment thread 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__)):

ilevkivskyi May 3, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

JelleZijlstra May 3, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

ilevkivskyi May 3, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes, a separate PR for 3.6 is needed. But I think it makes sense to already fix the async version here.

JelleZijlstra May 4, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Filed http://bugs.python.org/issue30266 to track that.

gvanrossum commented Jun 9, 2017

Copy link
Copy Markdown
Member

Wait, isn't this a new feature that should go into 3.7?

ilevkivskyi commented Jun 9, 2017

Copy link
Copy Markdown
Member

Wait, isn't this a new feature that should go into 3.7?

Oh yes, sorry, this one is not urgent then.

JelleZijlstra mentioned this pull request Jun 9, 2017
Merged
JelleZijlstra added a commit to JelleZijlstra/typeshed that referenced this pull request Jun 24, 2017
add typing.AsyncContextManager and contextlib.asynccontextmanager
32977e0 
Implements:
- python/typing#438
- python/cpython#360

python/cpython#1412, which adds
contextlib.AbstractAsyncContextManager, has not yet been merged.
JelleZijlstra mentioned this pull request Jun 24, 2017
Merged
gvanrossum pushed a commit to python/typeshed that referenced this pull request Jun 27, 2017
add typing.AsyncContextManager and contextlib.asynccontextmanager (#1432
22f47fd 
)

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

Copy link
Copy Markdown
Member

Please add a NEWS entry and I'll merge this in.

JelleZijlstra commented Oct 11, 2017

Copy link
Copy Markdown
Member Author

Thanks, just pushed a NEWS entry.

1st1 reviewed Oct 11, 2017
View reviewed changes
Comment thread Doc/library/contextlib.rst Outdated

.. class:: AbstractAsyncContextManager

An :term:`abstract base class` similar to

1st1 Oct 11, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Contributor

The PR #4790 adds AsyncExitStack subclass. It should inherit from AbstractAsyncContextManager.

ilevkivskyi commented Dec 13, 2017

Copy link
Copy Markdown
Member

@JelleZijlstra Could you please fix the merge conflict and update the docs as @1st1 proposed? I would like to have this merged soon.

1st1 merged commit 176baa3 into python:master Dec 14, 2017

1st1 commented Dec 14, 2017

Copy link
Copy Markdown
Member

Thanks @JelleZijlstra!

JelleZijlstra deleted the aacm branch December 14, 2017 01:28

JelleZijlstra commented Dec 14, 2017

Copy link
Copy Markdown
Member Author

Thanks, and sorry for dropping the ball on making the docs change!

1st1 commented Dec 14, 2017

Copy link
Copy Markdown
Member

NP ;)

ilevkivskyi commented Dec 14, 2017

Copy link
Copy Markdown
Member

Thank you @JelleZijlstra and @1st1 !

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1st1 1st1 approved these changes
ilevkivskyi ilevkivskyi approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL