Methods decorated with lazy are not included in ApiDocs

[Ryan J Ollos]

The following patch seems to fix the issue:

trac/util/__init__.py

@@ -20 +20 @@
 from __future__ import with_statement
 
 import errno
+import functools
 import inspect
 from itertools import izip, tee
 import locale
@@ -1109 +1110 @@
 
     def __init__(self, fn):
         self.fn = fn
+        functools.update_wrapper(self, fn)
 
     def __get__(self, instance, owner):
         if instance is None:

[Ryan J Ollos]

Also, previously we'd see:

$ python
Python 2.7.4 (default, Sep 26 2013, 03:20:26) 
[GCC 4.7.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from trac.env import Environment
>>> Environment.href.__doc__
'A lazily-evaluated attribute'
>>> Environment.abs_href.__doc__
'A lazily-evaluated attribute'

Now:

$ python
Python 2.7.4 (default, Sep 26 2013, 03:20:26) 
[GCC 4.7.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from trac.env import Environment
>>> Environment.href.__doc__
'The application root path'
>>> Environment.abs_href.__doc__
'The application URL'

Fixed in [12968], merged to trunk in [12969].

[Ryan J Ollos]

The cached and with_transaction decorators have the same issue. I'll fix it for cached, but will ignore with_transaction since it's deprecated. ​functools is only available since Python 2.5, which is why I didn't backport to Trac 0.12 (which supports Python 2.4).

[Ryan J Ollos]

Proposed fix for cached decorator:

trac/cache.py

@@ -13 +13 @@
 
 from __future__ import with_statement
 
+import functools
+
 from .core import Component
 from .util import arity
 from .util.concurrency import ThreadLocal, threading
@@ -173 +175 @@
         supported (but will be removed in version 1.1.1).
     """
     if hasattr(fn_or_attr, '__call__'):
-        return CachedSingletonProperty(fn_or_attr)
+        property = CachedSingletonProperty(fn_or_attr)
+        functools.update_wrapper(property, fn_or_attr)
+        return property
     def decorator(fn):
-        return CachedProperty(fn, fn_or_attr)
+        property = CachedProperty(fn, fn_or_attr)
+        functools.update_wrapper(property, fn)
+        return property
     return decorator

[Jun Omae]

Replying to rjollos:

The cached and with_transaction decorators have the same issue.

I'm not sure about cached decorator. The decorator updates __doc__ property from the original function in tags/trac-1.0.1/trac/cache.py@:42#L37​.

>>> from trac.ticket.api import TicketSystem
>>> TicketSystem.fields
<trac.cache.CachedSingletonProperty object at 0x28b99d0>
>>> TicketSystem.fields.__doc__
'Return the list of fields available for tickets.'
>>>

[Ryan J Ollos]

I didn't notice the assignment to __doc__, but there is still a problem. The command help(TicketSystem) yields:

...
 |  __init__(self)
 |  
 |  custom_fields = <trac.cache.CachedSingletonProperty object>
 |  eventually_restrict_owner(self, field, ticket=None)
 |      Restrict given owner field to be a list of users having
 |      the TICKET_MODIFY permission (for the given ticket)
 |  
 |  fields = <trac.cache.CachedSingletonProperty object>
 |  format_summary(self, summary, status=None, resolution=None, type=None)
...

With the use of functools.wrapped_function:

...
 |  
 |  __init__(self)
 |  
 |  custom_fields(...)
 |      Return the list of custom ticket fields available for tickets.
 |  
 |  eventually_restrict_owner(self, field, ticket=None)
 |      Restrict given owner field to be a list of users having
 |      the TICKET_MODIFY permission (for the given ticket)
 |  
 |  fields(...)
 |      Return the list of fields available for tickets.
 |  
...

That's not quite right though because it suggests that fields and custom_fields are functions rather than attributes. Methods decorated with lazy have the same issue.

Using functools.wrapped_function would fix this issue:

>>> TicketSystem.fields.__module__
'trac.cache'

—>

>>> TicketSystem.fields.__module__
'trac.ticket.api'

The __dict__ is different too:

>>> TicketSystem.fields.__dict__
{'__doc__': 'Return the list of fields available for tickets.', 'retriever': <function fields at 0x3165050>}

When using functools.wrapped_function:

>>> TicketSystem.fields.__dict__
{'__name__': 'fields', '__module__': 'trac.ticket.api', '__doc__': 'Return the list of fields available for tickets.', 'retriever': <function fields at 0x329ee60>}

We could just remove the assignment to __doc__ and use functool.wrapped_function, unless some issue is found with that approach. The docstring was added in [8071/trunk/trac/cache.py] (or at least the class was moved in that revision). Python 2.4 was supported at that time, so it looks like functools.wrapped_function could not be used directly from the standard library.

[Jun Omae]

Thanks for the details. I understand it.

That's not quite right though because it suggests that fields and custom_fields are functions rather than attributes. Methods decorated with lazy have the same issue.

Adding __set__ method, as a result the names would be listed as a attribute.

Also, _ isn't used in message for AttributeErrors. That's intentional. I don't think that messages should be translated because it is lead by programming error and would be logged to trac.log. Users don't see it in users' browser.

trac/cache.py

@@ -13 +13 @@
 
 from __future__ import with_statement
 
+import functools
+
 from .core import Component
 from .util import arity
 from .util.concurrency import ThreadLocal, threading
@@ -39 +41 @@
 
     def __init__(self, retriever):
         self.retriever = retriever
-        self.__doc__ = retriever.__doc__
+        functools.update_wrapper(self, retriever)
 
     def make_key(self, cls):
         attr = self.retriever.__name__
@@ -49 +51 @@
                 break
         return '%s.%s.%s' % (cls.__module__, cls.__name__, attr)
 
+    def __set__(self, instance, value):
+        raise AttributeError('Setting attribute is not allowed.')
+
 
 class CachedSingletonProperty(CachedPropertyBase):
     """Cached property descriptor for classes behaving as singletons

Edit: removed __set__ from lazy class in the patch. Cannot create TicketSystem instance with __set__.

[Ryan J Ollos]

I'm a bit unsure, but extrapolating on the changes from comment:6, I tried inheriting from property. Proposed changes in log:rjollos.git:t11684.2.

[Jun Omae]

Replying to rjollos:

Proposed changes in log:rjollos.git:t11684.2.

Looks good to me.

[Ryan J Ollos]

I'm fairly confident that [1b87dcef/rjollos.git] is okay. Inheriting from property adds an AttributeError-raising __set__ method, which as far as I understand it would be effectively the same as comment:6.

I'm less certain about [008a4635/rjollos.git]. Removing the inheritance from property, the tests still pass and the documentation displays the property as a data description rather than a method: [0d28fb30/rjollos.git].

We could make lazy act as a lazily evaluated property: [265d37a8/rjollos.git], however it is probably safer (for backward compatibility) to have it act as an attribute and to add a lazyproperty class if needed.

I'm leaning towards committing [1b87dcef/rjollos.git] and creating a new ticket for discussion of the other changes in 1.0.3.

[Ryan J Ollos]

[1b87dcef/rjollos.git] committed in [13028], merged in [13029]. #11693 created for changes discussed in comment:11.