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

[Kevin Smith]

Thanks for the great feed back. Hopefully I can get to it within the 
next few days.

Martijn Faassen wrote:
> 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.
I believe IDefaultBrowserLayer is used as a base interface, but since 
it's tied into Rotterdam I'd rather not, with the goal to remove 
Rotterdam and possibly replace it with IMinimalLayer or something more 
explicitly built-into and available to grok.
>
> 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?
Good catch, it's can be used in context of both a class and module, I'll 
update interfaces.py
>
> 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.
Sounds good. In general, the ViewGrokker seems a bit complex whereas the 
other grokkers are quite elegant.
>
> Anyway, only minor comments. With those changes, please, please merge! 
> +100!
>
> You appear to be missing in the CREDITS.txt file! Add yourself, 
> please. :)
Cool.
>
> Regards,
>
> Martijn
>
> _______________________________________________
> Grok-dev mailing list
> Grok-dev at zope.org
> http://mail.zope.org/mailman/listinfo/grok-dev
>