K, I just uploaded another patch to ticket #6262. Comments inline.
On Mon, Nov 16, 2009 at 5:58 AM, Russell Keith-Magee
<freakboy3...@gmail.com> wrote:
> On Thu, Nov 12, 2009 at 9:15 AM, Mike Malone <mjmal...@gmail.com> 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.
Good catch. Fixed in the new patch. I also added a test since this
functionality didn't seem to be covered.
> * 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)
Interesting idea. I had mixed feelings about the import. I'd rather
have this than a new setting.
The only problem here is that it's a bit tricky to tell whether you're
using the old style (a string specifying a callable) or the new style
(a string specifying a module that has a 'Loader' class). I've
implemented this in the new patch, and it seems to work, but it could
probably use a code review. If we're all in agreement I'll go ahead
and update the docs to reflect this new style.
> * 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.
Yea, I wasn't sure about including that example. I wrote it up as a
proof of concept, but it probably belongs somewhere as a blog post
instead of being in the official docs.
> * 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
Included in the latest patch.
> * 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.
Made find_template_source wrap find_template. It raises an exception
if a compiled template (something with a 'render' method) is found.
Mike
--
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email to django-develop...@googlegroups.com.
To unsubscribe from this group, send email to
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=.
