This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author yselivanov
Recipients asvetlov, gvanrossum, ncoghlan, scoder, vstinner, yselivanov
Date 2015-05-10.16:21:44
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1431274904.9.0.651865353913.issue24017@psf.upfronthosting.co.za>
In-reply-to
Content
> Review sent - very nice work on this Yury.

Thanks a lot, Nick!

Highlights:

> * I concur with Stefan that we should have a full PyCoroutineMethods struct at the C level, with a "tp_as_coroutine" pointer to that replacing the current tp_reserved slot

Do you think that tp_as_async is a better name?  (I explained my point of view in code review comments)

Also, do we need slots for __aenter__ and __aexit__? We don't have slots for regular context manager protocol, fwiw.

> * I also concur with Stefan about adding a Coroutine ABC

I will.  We definitely need it.

> * PyType_FromSpec (and typeslots.h) will need updating once we agree on a slot structure (with my recommendation being "define C level slots for all of the new PEP 492 methods")

> * I found CO_COROUTINE/CO_NATIVE_COROUTINE confusing as a reader of the implementation, as they only told me how the objects were defined, rather than telling me why I should care. Based on what I gleaned of their intended purpose from reading the implementation, I suggest switching this to instead use CO_COROUTINE (set for all coroutines, regardless of how they were defined) and CO_ITERABLE_COROUTINE (set only for those coroutines that also support iteration), and adjusting the naming of other APIs accordingly.

I agree that CO_COROUTINE is something that we should use for 'async def' functions (instead of CO_NATIVE_COROUTINE).  However, CO_ITERABLE_COROUTINE sounds a bit odd to me, as generator-based coroutines (at least in asyncio) aren't supposed to be iterated over.  How about CO_GENBASED_COROUTINE flag?


> * I found the names of the WITH_CLEANUP_ENTER and WITH_CLEANUP_EXIT bytecodes misleading, as they don't refer to the corresponding context management phases - they're both related to the "exit" phase. WITH_CLEANUP_START and WITH_CLEANUP_FINISH should be clearer for readers (both of the implementation and of the disassembled bytecode).

Big +1. Your names are much better.
History
Date User Action Args
2015-05-10 16:21:44yselivanovsetrecipients: + yselivanov, gvanrossum, ncoghlan, scoder, vstinner, asvetlov
2015-05-10 16:21:44yselivanovsetmessageid: <1431274904.9.0.651865353913.issue24017@psf.upfronthosting.co.za>
2015-05-10 16:21:44yselivanovlinkissue24017 messages
2015-05-10 16:21:44yselivanovcreate