On Thu, Nov 12, 2009 at 9:15 AM, Mike Malone <[email protected]> wrote:
> Sup,
>
> I've been working on template caching (ticket #6262) with a mister
> Alex Gaynor. The patch is getting pretty stable and I think it's close
> to merge-able, so if anyone wants to take a look at what we've got and
> provide feedback: go!
>
> Interesting background reading for people who haven't been part of
> this conversation:
> http://groups.google.com/group/django-developers/browse_thread/thread/b289b871285b86f5/b97ba9e2e9b9ad86
>
> Ticket: http://code.djangoproject.com/ticket/6262
>
> Latest patch:
> http://code.djangoproject.com/attachment/ticket/6262/cache_templates.5.diff
Hi Mike,
Here are my review comments. On the whole, I like what I see. These
are pretty much all fairly minor bugs, documentation, or cosmetic
interface changes.
* In the process of reversing the direction of the stack in Context,
the get() method has been neglected - it's still using the old stack
direction.
* The '.loader' extension on TEMPLATE_LOADER entries is consistent
with the old TEMPLATE_LOADER settings (i.e., pointing at a specific
instance/method), but not with the other pluggable backend APIs that
we have.
For example, when you specify the caching and mail backends, you
provide the name of the module, and it is assumed that the backend
module will contain a CacheClass and EmailBackend class, respectively.
For the sake of consistency and clarity, I'd rather see the 'loader'
name suffix as the implied (and required) name for the object in the
loader module, rather than needing to explicitly specify the name of
the loader instance.
Obviously, the legacy support for the get_template_loader function
will need to be an exception here, but moving forward, we should be
aiming at a clean API.
* On the subject of specifying loaders - in order to use the cached
loader, we need to import the cached loader, and have support for
callable loaders in find_template_loader. Requiring imports in the
settings file seems like a bit of a wart to me.
Here's an alternate proposal - rather than allowing callables, how
about allowing entries in TEMPLATE_LADERS to be tuples, and
interpreting the [1:] elements of the tuple as the arguments to the
loader - for example:
TEMPLATE_LOADERS = (
('django.template.loaders.cached', (
'django.template.loaders.filesystem.loader',
'django.template.loaders.app_directories.loader',
)
)
)
Theoretically, this means we could do away with the TEMPLATE_DIRS
setting, since we could specify template directories in the same
fashion. In practice, this probably isn't worth doing, but it is worth
pointing out as a possibility.
This also means we could get away from needing a module-level
instantiation of Loader() objects - you just look in the module for
the Loader class, and find_template_loader instantiates it with the
appropriate arguments (or no arguments if the loader is specified as a
string, rather than a tuple)
* As someone who will need to maintain this documentation over time,
I don't want to have to keep abreast of changes in other template
languages to ensure that Django's documentation is accurate. I have no
problems with mentioning Jinja and Cheetah as other languages that
could be supported in theory, but I'd rather not give the specific
implementation example.
* Also on documentation - the load_template_source methods should be
mentioned in internals/deprecation.txt, and the cached templates
feature should noted in the 1.2 release notes.
* The load_template_source methods should raise a PendingDeprecationWarning
* Lastly - and this is really the only hairy one - the fate of
django.template.loader.find_template_source().
This method isn't documented publicly, but the backwards-compatibility
notes provide that "everything documented in the linked documents
below, and all methods that don’t begin with an underscore – will not
be moved or renamed without providing backwards-compatible aliases."
It then goes on to specifically mention template APIs as a stable
area. By my reading, this means we can't just rename the method.
However, I think there is a fairly simple fix. Keep the new
find_template() method as is, but change find_template_source to be a
wrapper around get_template() that raises a PendingDeprecationWarning,
and raises a full exception if the template that is found is already
compiled (i.e., has a render method). This means that you won't be
able to use a cached template loader if your code is dependent on
find_template_source, but it also means that all existing uses of
find_template_source should continue to work.
*****
Other than these fairly minor bits, I'm happy. However, I would like
to see at least one other set of eyeballs giving a review of this
patch before commit - in particular, to the changes around BlockNode
and ExtendsNode. These are the bits that are the biggest changes
conceptually, and while I can't see any problems, I'd prefer to get a
sanity check from someone else in the core.
Yours,
Russ Magee %-)
--
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=.
