On Mon, Nov 16, 2009 at 8: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.
>
> * 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',
> )
> )
> )
>
I think this could get fairly complicated quickly, for example is this valid?
TEMPLATE_LOADERS = (
('django.templaet.loaders.cached', (
('django.template.loaders.filesystem', (
'/home/alex/django/templates/',
)
)
)
Frankly it seems more trouble then it's worth, adding a
CACHED_TEMPLATE_LOADER setting, though vaguely un-dry seems a good
deal cleaner.
> 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 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=.
>
>
>
Alex
--
"I disapprove of what you say, but I will defend to the death your
right to say it." -- Voltaire
"The people's good is the highest law." -- Cicero
"Code can always be simpler than you think, but never as simple as you
want" -- Me
--
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=.
