GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-30241: implement contextlib.AbstractAsyncContextManager by JelleZijlstra · Pull Request #1412 · python/cpython

Conversation

@JelleZijlstra

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

https://bugs.python.org/issue30241

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@JelleZijlstra
implement contextlib.AbstractAsyncContextManager
b0f92e5
@mention-bot

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 3, 2017
ilevkivskyi
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 ilevkivskyi May 3, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@JelleZijlstra JelleZijlstra May 3, 2017

Copy link
Copy Markdown
Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ilevkivskyi ilevkivskyi May 3, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@JelleZijlstra JelleZijlstra May 4, 2017

Copy link
Copy Markdown
Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@JelleZijlstra
support the "= None" pattern
9fb1028
ilevkivskyi
ilevkivskyi approved these changes May 4, 2017
View reviewed changes
@gvanrossum

gvanrossum commented Jun 9, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ilevkivskyi

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JelleZijlstra JelleZijlstra mentioned this pull request Jun 9, 2017
Add AsyncContextManager generic class python/typing#438
Merged
JelleZijlstra added a commit to JelleZijlstra/typeshed that referenced this pull request Jun 24, 2017
@JelleZijlstra
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 JelleZijlstra mentioned this pull request Jun 24, 2017
add typing.AsyncContextManager and contextlib.asynccontextmanager python/typeshed#1432
Merged
gvanrossum pushed a commit to python/typeshed that referenced this pull request Jun 27, 2017
@JelleZijlstra @gvanrossum
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

1st1 commented Oct 10, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JelleZijlstra
add NEWS entry
290802f
@JelleZijlstra

JelleZijlstra commented Oct 11, 2017

Copy link
Copy Markdown
Member Author

Thanks, just pushed a NEWS entry.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
1st1
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 1st1 Oct 11, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Kentzo

Kentzo commented Dec 11, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ilevkivskyi

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JelleZijlstra
rewrite docs as suggested by @1st1
171c597
@JelleZijlstra
JelleZijlstra force-pushed the aacm branch from 0e63bee to 171c597 Compare December 14, 2017 00:52
JelleZijlstra added 2 commits December 13, 2017 16:53
@JelleZijlstra
Merge branch 'master' into aacm
2a2cf36
@JelleZijlstra
Merge branch 'master' into aacm
aaa2590
1st1
1st1 approved these changes Dec 14, 2017
View reviewed changes
@bedevere-bot bedevere-bot added the awaiting merge label Dec 14, 2017
@1st1
1st1 merged commit 176baa3 into python:master Dec 14, 2017
@bedevere-bot bedevere-bot removed the awaiting merge label Dec 14, 2017
@1st1

1st1 commented Dec 14, 2017

Copy link
Copy Markdown
Member

Thanks @JelleZijlstra!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JelleZijlstra
JelleZijlstra deleted the aacm branch December 14, 2017 01:28
@JelleZijlstra

JelleZijlstra commented Dec 14, 2017

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@1st1

1st1 commented Dec 14, 2017

Copy link
Copy Markdown
Member

NP ;)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ilevkivskyi

ilevkivskyi commented Dec 14, 2017

Copy link
Copy Markdown
Member

Thank you @JelleZijlstra and @1st1 !
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@JelleZijlstra @mention-bot @gvanrossum @ilevkivskyi @1st1 @Kentzo @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.