GeezerVison

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

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 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)

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

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.

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

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?

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.

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.

Thanks! Fixed the comment.

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 :)

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

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?

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

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?

@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

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

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

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

Thanks!

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

Yes, sounds good. I can do that.