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

[Tres Seaver]

On Thu, 2002-03-21 at 09:34, Florent Guillaume wrote:
> 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 ?

Nope;  I just aliased the import as 'ICachingPolicyManager'.

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

I have done both;  I find I like using tuples for such lists, as I
avoid a possible persistency bug.

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

I thought about that;  the usual reason to use 'aq_inner' is to
enforce that objects may only be looked up in the containment
hierarchy (esp. for security).  In this context, I am not sure I
want to disallow acquisition trickery.

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

Yep, that is an artifact of resolving a merge error.  Fixed.

Thanks!

Tres.
-- 
===============================================================
Tres Seaver                                tseaver@zope.com
Zope Corporation      "Zope Dealers"       http://www.zope.com