Opened 6 years ago

Closed 6 years ago

#7832 enhancement closed fixed (fixed)

twisted.python.components._ProxiedClassMethod could look more like a normal method object.

Reported by: Jean-Paul Calderone Owned by: Jean-Paul Calderone
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch:
Author:

Description

_ProxiedClassMethod implements part of the standard ... "method protocol" ... That is, it has a __call__ method. It's missing other things that a method typically has, though.

In particular, missing __name__ makes it impossible to use functools.wraps with one of these proxied things. The name information is readily available, it just gets put at the methodName attribute instead of the __name__ attribute.

>>> from twisted.python.components import proxyForInterface
>>> from twisted.internet.interfaces import IReactorTCP
>>> from functools import wraps
>>> @wraps(proxyForInterface(IReactorTCP).connectTCP)
... def f():
...     pass
... 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.7/functools.py", line 33, in update_wrapper
    setattr(wrapper, attr, getattr(wrapped, attr))
AttributeError: '_ProxiedClassMethod' object has no attribute '__name__'
>>> 

Encountered while writing a class decorator which applied a decorator to one of the decorated class's methods. This worked fine until I tried to decorate a class that used proxyForInterface.

Attachments (4)

7832.patch (2.0 KB) - added by Jean-Paul Calderone 6 years ago.
7832-wrong-docstring.patch (2.9 KB) - added by Chris Wolfe 6 years ago.
7832-2-wrong-docstring.patch (2.9 KB) - added by Chris Wolfe 6 years ago.
exarkun's patch, failing doc test, fixed spacing, remove u"..."
7832-3-wrong-docstring.patch (2.3 KB) - added by Chris Wolfe 6 years ago.
fixes whitespace errors I introduced - sorry for spam.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 6 years ago by Jean-Paul Calderone

Owner: set to Jean-Paul Calderone

Changed 6 years ago by Jean-Paul Calderone

Attachment: 7832.patch added

comment:2 Changed 6 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted

comment:3 Changed 6 years ago by Jean-Paul Calderone

Development sponsored by ClusterHQ Inc.

comment:4 Changed 6 years ago by Wilfredo Sánchez Vega

Keywords: review removed
Owner: set to Jean-Paul Calderone

Unclear whether the docstring should document __name__… I guess that's a known thing for methods to have, so that would be redundant?

Since the class _ProxiedClassMethod is private… are it's ivars considered public API? If the methodName ivar is only used internally, renaming it rather than copying it seems sensible.

These two seem fuzzy to me, so I'll defer to exarkun on them. As is, this change is sound, I think.

Changed 6 years ago by Chris Wolfe

Attachment: 7832-wrong-docstring.patch added

comment:5 Changed 6 years ago by Chris Wolfe

exarkun,

It looks like the __doc__ attribute is incorrect as well.

With your patch applied, the decorated method can indeed be wrapped and called. However, when that method's f.doc attribute is accessed the docstring for t.w.c.proxyForInterface is returned.

I've attached a patch, https://twistedmatrix.com/trac/attachment/ticket/7832/7832-3-wrong-docstring.patch, containing your changes and a failing test showing that the docstring returned does not match the docstring of the function being wrapped.

Thanks!

(sorry for spamming the ticket with multiple patches - I introduced whitespace errors that I did not notice until it was too late).

Last edited 6 years ago by Chris Wolfe (previous) (diff)

Changed 6 years ago by Chris Wolfe

exarkun's patch, failing doc test, fixed spacing, remove u"..."

Changed 6 years ago by Chris Wolfe

fixes whitespace errors I introduced - sorry for spam.

comment:6 Changed 6 years ago by Jean-Paul Calderone

Thanks for the additional fixes herrwolfe. I'm reluctant to include them here as I don't personally have a requirement for the __doc__ behavior right now. It does seem like it is probably an improvement but perhaps it can be handled as part of a separate ticket. (Also, I notice the new test method name doesn't follow the Twisted naming convention.)

Unclear whether the docstring should document name… I guess that's a known thing for methods to have, so that would be redundant?

Probably doesn't hurt to be explicit. I'll add it.

Since the class _ProxiedClassMethod is private… are it's ivars considered public API? If the methodName ivar is only used internally, renaming it rather than copying it seems sensible.

They are public unfortunately because you can reach them without ever using a private API. For example, proxyForInterface(IFoo)(foo).foo_method.methodName. So we'll have to stick with being redundant.

I'll make documentation change and then apply this to trunk. Thanks!

comment:7 Changed 6 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [44292]) Apply 7832.patch with __name__ documentation added.

Author: exarkun Reviewer: wsanchez Fixes: #7832

Give proxied methods created by proxyForInterface a __name__ attribute as a normal method would have.

Note: See TracTickets for help on using tickets.