Comment 2 for bug 348267

Revision history for this message
nick (nick-bower) wrote :

Thanks Stephan but I have not seen this implict assumption mentioned anywhere in either z3c.form or zope.app.pagetemplate interfaces, api, docs or tests. Sure it would probably work now reading the code but this should be explicit behvaiour enforced with a type assertion in the render method.

In fact BoundPageTemplate, now that I'm looking at it, isn't even a PageTemplate subclass, so it looks like we're using the same template attribute for either an IPageTemplate implementation or an untyped proxy object. I've got issues with that - Python may be untyped but that doesn't make this good practise anywhere.

Even the BoundPageTemplate API doesn't suggest what you've said and what it's actually used for, *args being unenforced:

    def __call__(self, *args, **kw):
        if self.im_self is None:
            im_self, args = args[0], args[1:]
        else:
            im_self = self.im_self
        return self.im_func(im_self, *args, **kw)

So I suggest 2 changes.

1) In the absense of BoundPageTemplate implementing a marker interface, I suggest changing render to at least include a type assertion to make it explicit and obvious (if we are really going to tolerate this attribute multi-typing).

    def render(self):
        '''See interfaces.IForm'''
        # render content template
        if self.template is None:
            template = zope.component.getMultiAdapter((self, self.request),
                IPageTemplate)
            return template(self)
        else:
            assert type(self.template) is type(BoundPageTemplate)
            return self.template()

2) In IForm, change the docstring to sometihng useful

    def render():
        '''Render the form, returning either acquired IPageTemplate implementation or an assigned BoundPageTemplate instance.'''