[Zope-CMF] Re: [CMF-checkins] CVS: CMF/CMFCore - CachingPolicyManager.py:1.2 FSPageTemplate.py:1.7 __init__.py:1.15

[Florent Guillaume]

Some nitpicks, as usual :-)

Tres Seaver  <tseaver@zope.com> wrote:
> Log Message:
>   - Added CachingPolicyManager tool, which manages caching policies
>     for skin methods, and updated FSPageTemplate to use a CPM if
>     found.
>
> === CMF/CMFCore/CachingPolicyManager.py 1.1 => 1.2 ===
[...]
> +class CachingPolicyManager( SimpleItem ):
> +    """
> +        Manage the set of CachingPolicy objects for the site;  dispatch
> +        to them from skin methods.
> +    """
> +
> +    __implements__ = CachingPolicyManager

Is the name equality intentional ?

> +    def _reorderPolicy( self, policy_id, newIndex ):
[...]
> +        idlist = list( self._policy_ids )
> +        ndx = idlist.index( policy_id )
> +        pred = idlist[ ndx ]
> +        idlist = idlist[ :ndx ] + idlist[ ndx+1: ]
> +        idlist.insert( newIndex, pred )
> +        self._policy_ids = tuple( idlist )

I'd write it as:
        ndx = idlist.index(policy_id)
        pred = idlist[ndx]
        del idlist[ndx]
And I'd store it as a list in _policy_ids to avoid list() and tuple()
conversions, but maybe this has other impacts I don't know about.


> === CMF/CMFCore/FSPageTemplate.py 1.6 => 1.7 ===
[...]
>  class FSPageTemplate(FSObject, Script, PageTemplate):
[...]
> +                    content = aq_parent( self )

The usual idiom I'm used to is aq_parent(aq_inner(self)).

Which reminds me I've always wanted an aq_container that does that
automatically :-)

[...]
> +            return FSPageTemplate.inheritedAttribute('pt_render')( self,
> +                    source, extra_context )
>              return FSPageTemplate.inheritedAttribute('pt_render')(
>                  self, source, extra_context )

Two of them is really overdoing things :-)


Florent
-- 
Florent Guillaume, Nuxeo (Paris, France)
+33 1 40 33 79 10  http://nuxeo.com  mailto:fg@nuxeo.com