Hi Preston, Thanks for your continued work on this and sorry for my slow answers.
On 30 janv. 2015, at 03:24, Preston Timmons <[email protected]> wrote: > The cache algorithm I added uses the origin object as a cache key. The "name" > attribute is used as part of the key value. Therefore, the underlying loader > must be sure to set name to a unique value per template. Indeed, since you added skipping, you can’t use template_name (the value passed to get_template()) as the cache key. > Can you think of a loader where we wouldn't want to require a unique name? Well, given what I just said above, we don’t have a choice here. We must make this a requirement of the template loader API. > Does that makes sense, or should I look for a way to incorporate that > into LoaderOrigin? Looking at the implementation, it feels weird to add attributes to the origin that are only used by the loader… If origins were dumb bags of data this would be all right but they have a reload() method that calls the loader. Such mutual dependencies never end well :-/ That’s a broader design problem not directly related to your refactoring, but while you’re breaking all the things, perhaps we could fix it too. > LoaderOrigin vs StringOrigin > > I looked at combining these, but didn't yet since it doesn't impact the > feature > this patch is implementing. If they're combined to a DTLOrigin, should that > live in django.template.base? It can't live in django.template.loader since > that module isn't safe for django.template.base to import. Yes, it can go into django.template.base or django.template.origin, a new module. By the way, I would like to check if we can deprecate the dirs argument of LoaderOrigin. It may only be used by deprecated code as of Django 1.8. > context.origin > > In order for the origin to be available to the ExtendsNode, I also used the > context.origin = self.origin hack in the Context.render method. I saw the > logic > you used to only set engine on toplevel_render. When I moved this assignment > into the if statement or not, it didn't seem to make a difference. > > Is that a hack we can live with for now? Well this isn’t an example you’re supposed to follow… ;-) context.origin doesn’t make a lot of sense. Wouldn’t context.template be better? Then you could walk to context.template.origin. In fact context.template.engine would be much better than context.engine. I’ll try to fix that in 1.8 before it’s too late. > There's one case where the debug postmortem isn't optimal. If multiple engines > are configured and a template is found by the second engine that fails > extending, the postmortem won't include the templates that were tried by the > initial get_template call to the first engine. I'm not seeing an obvious way > to persist that information at the moment. This isn’t a big deal. Using multiple Django template engines is an edge case. -- Aymeric. -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at http://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/1D3E35AF-675D-4C00-B463-85D968D39A43%40polytechnique.org. For more options, visit https://groups.google.com/d/optout.
