Edgewall Software
Modify

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#11684 closed defect (fixed)

Methods decorated with lazy are not included in ApiDocs

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.0.2
Component: general Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:
API Changes:

The decorators lazy and cached expose the method docstring in documentation displayed with the built-in help. The classes that implement the cached decorator (CachedProperty and CachedSingletonProperty) were modified so that the decorated attributes look like data descriptors.

Internal Changes:

Description

The following patch seems to fix the issue:

  • trac/util/__init__.py

    diff --git a/trac/util/__init__.py b/trac/util/__init__.py
    index 13676d5..a31a147 100644
    a b  
    2020from __future__ import with_statement
    2121
    2222import errno
     23import functools
    2324import inspect
    2425from itertools import izip, tee
    2526import locale
    class lazy(object):  
    11091110
    11101111    def __init__(self, fn):
    11111112        self.fn = fn
     1113        functools.update_wrapper(self, fn)
    11121114
    11131115    def __get__(self, instance, owner):
    11141116        if instance is None:

Attachments (0)

Change History (13)

comment:1 by Ryan J Ollos, 10 years ago

API Changes: modified (diff)
Resolution: fixed
Status: newclosed

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].

comment:2 by Ryan J Ollos, 10 years ago

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).

comment:3 by Ryan J Ollos, 10 years ago

Proposed fix for cached decorator:

  • trac/cache.py

    diff --git a/trac/cache.py b/trac/cache.py
    index 711e2b8..2bee647 100644
    a b  
    1313
    1414from __future__ import with_statement
    1515
     16import functools
     17
    1618from .core import Component
    1719from .util import arity
    1820from .util.concurrency import ThreadLocal, threading
    def cached(fn_or_attr=None):  
    173175        supported (but will be removed in version 1.1.1).
    174176    """
    175177    if hasattr(fn_or_attr, '__call__'):
    176         return CachedSingletonProperty(fn_or_attr)
     178        property = CachedSingletonProperty(fn_or_attr)
     179        functools.update_wrapper(property, fn_or_attr)
     180        return property
    177181    def decorator(fn):
    178         return CachedProperty(fn, fn_or_attr)
     182        property = CachedProperty(fn, fn_or_attr)
     183        functools.update_wrapper(property, fn)
     184        return property
    179185    return decorator

in reply to:  2 comment:4 by Jun Omae, 10 years ago

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.'
>>>

comment:5 by Ryan J Ollos, 10 years ago

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.

Last edited 10 years ago by Ryan J Ollos (previous) (diff)

in reply to:  5 comment:6 by Jun Omae, 10 years ago

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

    diff --git a/trac/cache.py b/trac/cache.py
    index 711e2b8..bd6afb6 100644
    a b  
    1313
    1414from __future__ import with_statement
    1515
     16import functools
     17
    1618from .core import Component
    1719from .util import arity
    1820from .util.concurrency import ThreadLocal, threading
    class CachedPropertyBase(object):  
    3941
    4042    def __init__(self, retriever):
    4143        self.retriever = retriever
    42         self.__doc__ = retriever.__doc__
     44        functools.update_wrapper(self, retriever)
    4345
    4446    def make_key(self, cls):
    4547        attr = self.retriever.__name__
    class CachedSingletonProperty(CachedPropertyBase):  
    7779            id = self.id = key_to_id(self.make_key(instance.__class__))
    7880        CacheManager(instance.env).invalidate(id)
    7981
     82    def __set__(self, instance, value):
     83        raise AttributeError('Setting attribute is not allowed.')
     84
    8085
    8186class CachedProperty(CachedPropertyBase):
    8287    """Cached property descriptor for classes having potentially
    class CachedProperty(CachedPropertyBase):  
    111116            setattr(instance, self.key_attr, id)
    112117        CacheManager(instance.env).invalidate(id)
    113118
     119    def __set__(self, instance, value):
     120        raise AttributeError('Setting attribute is not allowed.')
     121
    114122
    115123def cached(fn_or_attr=None):
    116124    """Method decorator creating a cached attribute from a data
  • trac/util/__init__.py

    diff --git a/trac/util/__init__.py b/trac/util/__init__.py
    index c0b81c2..1612a85 100644
    a b class lazy(object):  
    11141114        setattr(instance, self.fn.__name__, result)
    11151115        return result
    11161116
     1117    def __set__(self, instance, value):
     1118        raise AttributeError('Setting attribute is not allowed.')
     1119
    11171120
    11181121# -- algorithmic utilities
    11191122
Version 0, edited 10 years ago by Jun Omae (next)

comment:7 by Ryan J Ollos, 10 years ago

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.

comment:8 by Ryan J Ollos, 10 years ago

Resolution: fixed
Status: closedreopened

in reply to:  7 comment:9 by Jun Omae, 10 years ago

Replying to rjollos:

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

Looks good to me.

comment:10 by Ryan J Ollos, 10 years ago

Status: reopenedassigned

comment:11 by Ryan J Ollos, 10 years ago

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.

comment:12 by Ryan J Ollos, 10 years ago

API Changes: modified (diff)
Resolution: fixed
Status: assignedclosed

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

comment:13 by Ryan J Ollos, 10 years ago

API Changes: modified (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.