◐ Shell
reader mode source ↗
Skip to content

bpo-29679: Implement @contextlib.asynccontextmanager#360

Merged
1st1 merged 18 commits into
python:masterfrom
JelleZijlstra:asynccontextmanager
May 1, 2017
Merged

bpo-29679: Implement @contextlib.asynccontextmanager#360
1st1 merged 18 commits into
python:masterfrom
JelleZijlstra:asynccontextmanager

Conversation

@JelleZijlstra

Copy link
Copy Markdown
Member

No description provided.

@mention-bot

Copy link
Copy Markdown

@JelleZijlstra, thanks for your PR! By analyzing the history of the files in this pull request, we identified @gvanrossum, @brettcannon and @Yhg1s to be potential reviewers.

@JelleZijlstra

Copy link
Copy Markdown
Member Author

Also adding @ncoghlan who I believe maintains contextlib (although I can't find module maintainers listed anywhere in the devguide or the repo). Not sure what prompted the error in mention-bot's first message.

JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull request Mar 1, 2017
@ncoghlan

ncoghlan commented Mar 1, 2017

Copy link
Copy Markdown
Contributor

@JelleZijlstra The maintainer list is at https://docs.python.org/devguide/experts.html#experts (you can also just start typing the module name into the nosy list field on bugs.python.org and it will autopopulate the dropdown with the relevant usernames)

@ncoghlan ncoghlan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

General concept and approach looks good to me, just some specific feedback on particular aspects of the implementation and test layout.

@JelleZijlstra

Copy link
Copy Markdown
Member Author

Thanks for reviewing so quickly! I've pushed fixes for the issues you raised.

@ncoghlan ncoghlan self-assigned this Mar 1, 2017
@ncoghlan

ncoghlan commented Mar 1, 2017

Copy link
Copy Markdown
Contributor

The PR itself looks good to me now, but I'm going to place it on hold for the moment pending further consideration of whether we should add this to contextlib itself, or introduce a new asyncio.contextlib module.

@ilevkivskyi

Copy link
Copy Markdown
Member

@1st1 is someone who might be interested in this, see for example the discussion of asyncio.run_forever() here python/asyncio#465

@ncoghlan

ncoghlan commented Mar 2, 2017

Copy link
Copy Markdown
Contributor

OK, based on the python-dev discussion, I'm happy with the idea of proceeding with this basic API design: http://bugs.python.org/issue29679#msg288781

However, those code coverage results are suspicious, as they suggest that either the async-based tests aren't actually running properly, or else the coverage report isn't capturing their execution correctly.

Could you take another look at those tests and inject some deliberate failures (I like 1/0) to make sure they're actually being executed by the test harness?

@JelleZijlstra

Copy link
Copy Markdown
Member Author

That's strange. When I change the code locally to make one of the tests fail, they definitely do fail. (I ran ./python.exe -m test -uall test_contextlib_async.)

The detailed Travis results show that test_contextlib_async failed in coverage mode (together with test_traceback and test_xml_etree), but doesn't give details. Maybe coverage implements async-related bytecode incompletely or something; I'll run the command on a local clone to figure out what is going on.

@JelleZijlstra

Copy link
Copy Markdown
Member Author

Found the issue and pushed a fix. The problem was that test_asyncio closes the default event loop, so if test_asyncio and test_contextlib_async are run consecutively in the same process, the contextlib_async tests that rely on get_event_loop() returning a usable loop fail. I fixed the problem by instead creating a new event loop. ./python.exe -m test test_asyncio test_contextlib_async failed before this fix and now passes.

@ncoghlan ncoghlan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

Noted several lines from the diff coverage report that the new test cases should really be hitting. The other uncovered lines all step from contextlib being imported being the coverage data starts being collected, so there isn't a lot to be done about that.

Feel free to ask for more info if it isn't clear how to craft a test case that covers the commented on lines.

29 hidden items Load more…
@JelleZijlstra

Copy link
Copy Markdown
Member Author

Thanks! Fixed the comment.

@ncoghlan

ncoghlan commented Mar 3, 2017

Copy link
Copy Markdown
Contributor

If you'd like, you can add a note to Docs/whatsnew/3.7.rst about the addition, otherwise I'll add it when I update Misc/NEWS prior to merging :)

@1st1

1st1 commented Mar 3, 2017

Copy link
Copy Markdown
Member

I've left another couple of comments in the review.

@1st1 1st1 left a comment

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.

Hide comment

LGTM. FWIW I'm really glad the tests didn't expose any bugs in asynchronous generators :)

@sashgorokhov

Copy link
Copy Markdown

Created the same package that implements asynccontextmanager decorator on these weekends, and wanted to push it into python. And here it is!

Any thoughts in which python release this will be included?

@ilevkivskyi

Copy link
Copy Markdown
Member

asyncio is no more provisional, and contextlib was never provisional, so that it looks like 3.7 is the earliest option.

@1st1

1st1 commented Mar 7, 2017

Copy link
Copy Markdown
Member

asyncio is no more provisional, and contextlib was never provisional, so that it looks like 3.7 is the earliest option.

Correct.

@ncoghlan do you want me to merge this PR?

@ncoghlan

ncoghlan commented Mar 8, 2017

Copy link
Copy Markdown
Contributor

@1st1 Go ahead, thanks :)

For folks interested in 3.5 and 3.6 support, I opened an issue against contextlib2 to discuss the options for handling that: jazzband/contextlib2#12

@JelleZijlstra

Copy link
Copy Markdown
Member Author

Not sure what happened to the Appveyor build, the failures don't look related to contextlib.

@JelleZijlstra

Copy link
Copy Markdown
Member Author

Is there anything blocking this from being merged? There were a few merge conflicts already.

@1st1

1st1 commented Apr 30, 2017

Copy link
Copy Markdown
Member

I'll merge it when it passes the checks. Thanks!

@JelleZijlstra

Copy link
Copy Markdown
Member Author

Thanks!

@1st1 1st1 merged commit 2e62469 into python:master May 1, 2017
@ilevkivskyi

Copy link
Copy Markdown
Member

@JelleZijlstra
I have another feature proposal: there is a class contextlib.AbstractContextManager and its typing generic counterpart. I would propose to add a very similar contextlib.AbstractAsyncContextManager (with __aenter__ and __aexit__). This will complete the picture and will be also useful in the context of PEP 544: Protocols. Are you interested in implementing this?

@JelleZijlstra JelleZijlstra deleted the asynccontextmanager branch May 1, 2017 13:35
@JelleZijlstra

Copy link
Copy Markdown
Member Author

Yes, sounds good. I can do that.

JelleZijlstra added a commit to JelleZijlstra/typeshed that referenced this pull request Jun 24, 2017
Implements:
- python/typing#438
- python/cpython#360

python/cpython#1412, which adds
contextlib.AbstractAsyncContextManager, has not yet been merged.
gvanrossum pushed a commit to python/typeshed that referenced this pull request Jun 27, 2017
)

Implements:
- python/typing#438
- python/cpython#360

Note that python/cpython#1412, which adds
contextlib.AbstractAsyncContextManager, has not yet been merged.
jaraco pushed a commit that referenced this pull request Dec 2, 2022
#360)

* Remove 'Automerge-Triggered-By' text when removing the automerge label

* Add more tests

* Add test case for the scenario that the trailer text does not exist
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants