bpo-29679: Implement @contextlib.asynccontextmanager#360
Conversation
|
@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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
@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) |
Sorry, something went wrong.
ncoghlan
left a comment
There was a problem hiding this comment.
General concept and approach looks good to me, just some specific feedback on particular aspects of the implementation and test layout.
Sorry, something went wrong.
|
Thanks for reviewing so quickly! I've pushed fixes for the issues you raised. |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
|
@1st1 is someone who might be interested in this, see for example the discussion of |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
|
That's strange. When I change the code locally to make one of the tests fail, they definitely do fail. (I ran The detailed Travis results show that |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
ncoghlan
left a comment
There was a problem hiding this 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.
Sorry, something went wrong.
|
Thanks! Fixed the comment. |
Sorry, something went wrong.
|
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 :) |
Sorry, something went wrong.
|
I've left another couple of comments in the review. |
Sorry, something went wrong.
1st1
left a comment
There was a problem hiding this comment.
LGTM. FWIW I'm really glad the tests didn't expose any bugs in asynchronous generators :)
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
|
Sorry, something went wrong.
Correct. @ncoghlan do you want me to merge this PR? |
Sorry, something went wrong.
|
@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 |
Sorry, something went wrong.
|
Not sure what happened to the Appveyor build, the failures don't look related to contextlib. |
Sorry, something went wrong.
|
Is there anything blocking this from being merged? There were a few merge conflicts already. |
Sorry, something went wrong.
|
I'll merge it when it passes the checks. Thanks! |
Sorry, something went wrong.
|
Thanks! |
Sorry, something went wrong.
|
@JelleZijlstra |
Sorry, something went wrong.
|
Yes, sounds good. I can do that. |
Sorry, something went wrong.
Implements: - python/typing#438 - python/cpython#360 python/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.
#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
No description provided.