[Grok-dev] regebro-guido-templates: comments

[Martijn Faassen]

Hi there,

I briefly looked at this branch last night. Overall I think the approach 
works well and I like how there are tests. :)

Here are some of my comments:

* We should seriously reconsider the naming_convention. Several methods 
are added that have underscores in them. Zope 3's camelCase and we tend 
to mostly follow those conventions for Grok, and this would break that. 
Of course if we can come up with good single word method names we can 
forgo this entire discussion.

* having to do self.__grok_module__ = martian.util.caller_module() 
everywhere in the Genshi integration isn't very nice. I wonder whether 
we can set this information externally during the grokking process so it 
won't put a constraint on subclassing.

* while the Genshi integration code is already short, it'd be nice if we 
could make it go down even more. Not very urgent before we merge this 
branch though - just something to consider.

* I don't like the way the namespace dictionary is filled, if I 
understand it correctly. Right now it reads like this:

namespace = view.default_namespace()
namespace.update(view.extra_namespace())

view.default_namespace() sets up request, view, context, and static in a 
dictionary
view.extra_namespace() returns an extra dictionary

This forces all template language implementations to work with this 
dictionary. There is no nice option to make a special genshi variety 
with a special extension (say, .gr for Genshi Restricted) that does this 
differently. 'request', 'view', 'context' and such will *always* be 
there, unless you go and override render_template() in the template plugin.

I think the responsibility for setting up the namespace should be 
primarily in the template language implementation. Therefore I'd suggest 
we move default_namespace() to the template language implementation, 
something like this:

def namespace(self, view):
     namespace = {}
     namespace['request'] = view.request
     namespace['view'] = view
     namespace['context'] = view.context
     namespace['static'] = view.static
     return namespace

Since we access view consistently here that indeed suggests the method 
should be on View. I think we're dealing with an exception here though, 
as the control needs to be with the template language. If we think we 
need to we can always introduce a helper method on the view to help set 
this up, but I think this is so simple it's not really required at all.

On the view we then also introduce a namespace method, which by default 
returns nothing at all:

def namespace(self):
    return {}

and the render_template() method in the template language extension does 
this:

namespace = self.namespace()
namespace.update(view.namespace())

A template language can then choose to let its namespace return an empty 
dictionary. In that case, the user of this template language will *have* 
to implement the namespace method on the view to do anything at all. 
Alternatively the writer of the template language plugin may choose to 
pass in more or different names into the basic namespace.

Note that incidentally I eliminated the naming discussion, as both these 
methods are called 'namespace()'. (following Brandon's suggestion in an 
earlier thread)

One override option that is available in the old design and not in this 
new design is the ability for the view author to *remove* values from 
the namespace. Thanks to the call to namespace.update(view.namespace()) 
overriding and extension will still work, but removing, say, 'request', 
from the namespace for a particular view won't work. I don't think this 
is a very useful feature - one typically expects all templates to have 
the same default namespace or none at all, and there is no need to 
adjust this on a per-view basis (unless it's done always for *all* the 
views).

Regards,

Martijn