pull request: issue #13 (3) "Remove requirement for templates to use views for partials"
- Chris Jerdonek
- 2011-12-02 @ 06:07
On the third pull request for issue #13--
I'm mixed on this one. For starters, this patch seems to do two things:
(1) use a template_loader inside Template.render_partial() instead of
a View instance, and
(2) pass an "encoding" parameter around to many of the Template
class's methods for Template.render_partial().
I think (2) might be unnecessary because internally, I think it might
be okay for the Template class to render partial templates to unicode
strings. It's only in the "outermost" call to Template.render() that
it might be necessary to encode to a given encoding. In fact, it
seems like it's possible that (2) could actually trigger a bug whereby
the Template class attempts to re-encode an already encoded byte
string (double encoding?).
Regarding (1), I think it might be premature to introduce passing a
template loader. For one, the code is different now so that a View
instance gets created in the Template's constructor no matter what.
So passing a template loader won't by itself fulfill the first part of
the overall goal of issue #13 which was "Remove view dependency from
templates and correctly load partials." It looks like before, the
render_partial() method was the only place where the View class was
used. Moreover, the second part of the title (correctly loading
partials) has already been taken care of in the development branch
Lastly, if a user wants to customize template loading, I believe they
can now do that by sub-classing View and overriding the
View.load_template() method which was just added in the development
branch. Given that the code has changed so much with respect to issue
#13, I think it may be worth waiting to see how the existing changes
fare. I also anticipate that future refactorings may further decouple
views and templates, so it might make more sense to revisit this issue
after that has been done.