Better representation of Handle and Task

Better representation of Handle and Task

[yselivanov, 2014-06-12 02:01:16]

besides few nits the patch looks good.

https://codereview.appspot.com/108910043/diff/1/asyncio/events.py
File asyncio/events.py (right):

https://codereview.appspot.com/108910043/diff/1/asyncio/events.py#newcode30
asyncio/events.py:30: if inspect.isfunction(self._callback):
what if we just try to access 'self._callback.__code__' without calling
isfunction?

https://codereview.appspot.com/108910043/diff/1/tests/test_events.py
File tests/test_events.py (right):

https://codereview.appspot.com/108910043/diff/1/tests/test_events.py#newcode1748
tests/test_events.py:1748: # mock.Mock()
what's the commented out stuff?

https://codereview.appspot.com/108910043/diff/1/tests/test_tasks.py
File tests/test_tasks.py (right):

https://codereview.appspot.com/108910043/diff/1/tests/test_tasks.py#newcode156
tests/test_tasks.py:156: # FIXME: check for the name "coro" because of "notmuch"
because
"because of .. because" -- this line is hard to parse

[haypo_gmail, 2014-06-12 08:49:44]

https://codereview.appspot.com/108910043/diff/1/asyncio/events.py
File asyncio/events.py (right):

https://codereview.appspot.com/108910043/diff/1/asyncio/events.py#newcode30
asyncio/events.py:30: if inspect.isfunction(self._callback):
On 2014/06/12 02:01:16, yselivanov wrote:
> what if we just try to access 'self._callback.__code__' without calling
> isfunction?

You get an attribute error. Try for example print.__code__ or
functools.partial(func).__code__

https://codereview.appspot.com/108910043/diff/1/tests/test_events.py
File tests/test_events.py (right):

https://codereview.appspot.com/108910043/diff/1/tests/test_events.py#newcode1748
tests/test_events.py:1748: # mock.Mock()
On 2014/06/12 02:01:16, yselivanov wrote:
> what's the commented out stuff?

Oh, they are no more needed. I removed them.

https://codereview.appspot.com/108910043/diff/1/tests/test_tasks.py
File tests/test_tasks.py (right):

https://codereview.appspot.com/108910043/diff/1/tests/test_tasks.py#newcode156
tests/test_tasks.py:156: # FIXME: check for the name "coro" because of "notmuch"
because
On 2014/06/12 02:01:16, yselivanov wrote:
> "because of .. because" -- this line is hard to parse

oops, fixed.

[yselivanov, 2014-06-12 14:24:35]

https://codereview.appspot.com/108910043/diff/1/asyncio/events.py
File asyncio/events.py (right):

https://codereview.appspot.com/108910043/diff/1/asyncio/events.py#newcode30
asyncio/events.py:30: if inspect.isfunction(self._callback):

On 2014/06/12 08:49:43, haypo_gmail wrote:
> On 2014/06/12 02:01:16, yselivanov wrote:
> > what if we just try to access 'self._callback.__code__' without calling
> > isfunction?
> 
> You get an attribute error. Try for example print.__code__ or
> functools.partial(func).__code__


Yes, that's what I meant:

code = getattr(func, '__code__', None)
if code:
    ... your code ...

[yselivanov, 2014-06-12 14:29:09]

https://codereview.appspot.com/108910043/diff/20001/asyncio/events.py
File asyncio/events.py (right):

https://codereview.appspot.com/108910043/diff/20001/asyncio/events.py#newcode24
asyncio/events.py:24: elif hasattr(func, '__wrapped__'):
It's a bit more complicated -- if a function is decorated multiple times, you
need to have a while loop to follow __wrapped__ chain. Doing it only once means
nothing.

[haypo_gmail, 2014-06-12 15:22:50]

https://codereview.appspot.com/108910043/diff/20001/asyncio/events.py
File asyncio/events.py (right):

https://codereview.appspot.com/108910043/diff/20001/asyncio/events.py#newcode24
asyncio/events.py:24: elif hasattr(func, '__wrapped__'):
On 2014/06/12 14:29:08, yselivanov wrote:
> It's a bit more complicated -- if a function is decorated multiple times, you
> need to have a while loop to follow __wrapped__ chain. Doing it only once
means
> nothing.

Are you sure?

In Python < 3.4, @functools.wraps doesn't create a chain:
https://docs.python.org/3.4/whatsnew/3.4.html#changes-in-the-python-api

"functools.update_wrapper() and functools.wraps() now correctly set the
__wrapped__ attribute to the function being wrapped, even if that function also
had its __wrapped__ attribute set. This means __wrapped__ attributes now
correctly link a stack of decorated functions rather than every __wrapped__
attribute in the chain referring to the innermost function. Introspection
libraries that assumed the previous behaviour was intentional can use
inspect.unwrap() to access the first function in the chain that has no
__wrapped__ attribute."

Do you know decorators which created such chain?

_get_function_code() is only used for debug purpose, it doesn't need to be
perfect. It can be enhanced later if needed.

[haypo_gmail, 2014-06-12 15:24:54]

https://codereview.appspot.com/108910043/diff/20001/asyncio/events.py
File asyncio/events.py (right):

https://codereview.appspot.com/108910043/diff/20001/asyncio/events.py#newcode49
asyncio/events.py:49: if inspect.isfunction(self._callback):
The code is different from the change set 1.

@Yury: Would you prefer something like that?

cb = getattr(self._callback, '__qualname__', None)
if not cb:
    cb = str(self._callback)

[yselivanov, 2014-06-12 15:29:18]

https://codereview.appspot.com/108910043/diff/20001/asyncio/events.py
File asyncio/events.py (right):

https://codereview.appspot.com/108910043/diff/20001/asyncio/events.py#newcode24
asyncio/events.py:24: elif hasattr(func, '__wrapped__'):
Oh.. I've always assumed it created a proper chain. Good to know this is finally
true in 3.4 ;)

https://codereview.appspot.com/108910043/diff/20001/asyncio/events.py#newcode49
asyncio/events.py:49: if inspect.isfunction(self._callback):
On 2014/06/12 15:24:54, haypo_gmail wrote:
> The code is different from the change set 1.
> 
> @Yury: Would you prefer something like that?
> 
> cb = getattr(self._callback, '__qualname__', None)
> if not cb:
>     cb = str(self._callback)

Yes, I think it reads better and is more future proof.

[haypo_gmail, 2014-06-12 16:10:49]

On 2014/06/12 15:29:18, yselivanov wrote:
> > @Yury: Would you prefer something like that?
> > 
> > cb = getattr(self._callback, '__qualname__', None)
> > if not cb:
> >     cb = str(self._callback)
> 
> Yes, I think it reads better and is more future proof.

Ok, it should handle more kinds of callable with getattr. Here is a new patch,
does it look good?

[yselivanov, 2014-06-12 16:17:17]

LGTM (one nit though)

https://codereview.appspot.com/108910043/diff/40001/asyncio/events.py
File asyncio/events.py (right):

https://codereview.appspot.com/108910043/diff/40001/asyncio/events.py#newcode49
asyncio/events.py:49: cb = getattr(self._callback, '__qualname__', None)
cb_repr would be a better name

[yselivanov, 2014-06-12 16:21:14]

https://codereview.appspot.com/108910043/diff/40001/asyncio/events.py
File asyncio/events.py (right):

https://codereview.appspot.com/108910043/diff/40001/asyncio/events.py#newcode49
asyncio/events.py:49: cb = getattr(self._callback, '__qualname__', None)
or cb_name