On Sun, May 10, 2015 at 7:21 PM, Yury Selivanov <report@bugs.python.org> wrote:
>
> Yury Selivanov added the comment:
>
>> 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?
>

Maybe CO_ASYNC_COROUTINE and CO_OLDSTYLE_COROUTINE?
This is wild proposal, feel free to ignore it.

>
>> * 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.
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue24017>
> _______________________________________