[Grok-dev] Re: Proposal to merge ksmith_mcweekly-layers branch

Martijn Faassen faassen at startifact.com
Tue Sep 18 15:54:46 EDT 2007


Kevin Smith wrote:
> Wow, alot *has* changed. Yes, please review, a new branch has been 
> created based on grok 0.11 trunk, at ksmith_mcweekly-layers-011. Ftests 
> are in view/layers.py

Thanks. I just took a look at the code. My feedback:

IGrokLayer is what we inherit from and import from grok. The interfaces 
documentation however doesn't describe IGrokLayer but instead talks 
about "Layer" as the base interface. I think we should be using IGrokLayer.

Does Zope 3 provide a ILayer base interface? If so, it might be 
worthwhile subclassing from it instead of just from interface.Interface, 
unless there are problems with this.

The 'grok.layer' directive has a ClassOrModuleDirectiveContext(). The 
interfaces.py documentation however claims that grok.layer can only be 
used in the context of a class. So which is it?

In meta.py, you have a rather complicated expression:

# grab layer, if there is one
         view_layer = util.class_annotation(factory, 'grok.layer',
                                            None) or 
module_info.getAnnotation('grok.layer',
                                                None) or 
IDefaultBrowserLayer

I'm the type of caveman who will have to scratch his head a few times to 
understand what's going down. I suggest rewriting this to a bunch of 
'if' statements instead. The same applies to the SkinGrokker. In both 
places you're looking for the class annotation and then if that fails 
fall back on the module annotation. Factor that into a single utility 
function, I'd suggest. The pattern may be common enough to place in 
martian.util.

Anyway, only minor comments. With those changes, please, please merge! +100!

You appear to be missing in the CREDITS.txt file! Add yourself, please. :)

Regards,

Martijn



More information about the Grok-dev mailing list