Hi Aymeric, Great work!
I'm afraid that my review comments are lengthy. Feel free to suggest that I provide them in some other format (issues and/or PRs on https://github.com/aaugustin/mtefd ?) if that would be more useful. * In the rationale section, it may be worth mentioning the specific case of template-driven form widgets, since this is a pathologically bad case for DTL performance (e.g. when rendering select-boxes with hundreds of choices) and (at least as far as I'm concerned) a strong motivator for a faster built-in templating option. * In the "selecting an engine" section, I think option 1, if the only API, has an additional downside beyond just boilerplate: it places control over engine selection at the rendering site, which may be outside the control of the project integrator. I still think it's good to provide option 1 as an option for tight low-level control in atypical cases. * I don't agree with the characterization of option 1 as "ugly" in the description of option 2: it's not ugly, it's a perfectly clean and sensible API. It's just low-level, less convenient, and doesn't offer override hooks for project integrators. (I would say option 2 is actually uglier than option 1, because it is conceptually incoherent given that template engines are responsible for their own template finding, loading, and parsing.) * The description for option 4 implies that ordering of template engines is only useful if their directories overlap. This is not the case. I would see ordering as perhaps most useful in cases where a project wants to selectively (or wholly) override some templates provided by a third-party app with replacements using a different engine. In this case, there would be no overlapping directories in the configuration, just the same template name provided in two different engines' directories. * In the current DTL configuration, the relative priority of `TEMPLATE_DIRS` vs app directories can be controlled via ordering of the `TEMPLATE_LOADERS` setting. In your proposed configuration scheme, how can one control the relative priority of `DIRS` vs `APP_DIRS`? This can be quite important for overriding templates provided by third-party apps. If that's the primary/only use case, perhaps it's sufficient to just have `DIRS` always take priority, since that's the more explicitly-configured option? (I see that this is what your sample `BaseEngine` implementation does - if that's your proposal, it should be more explicit in the DEP.) * Similarly, since it's left up to the user to configure other loading mechanisms that an engine might offer (presumably via OPTIONS), that raises the question of how a user could control the priority of those alternative loading mechanisms vs `DIRS` and `APP_DIRS`. Perhaps the best choice here is to punt that issue to the individual template engine and its OPTIONS. * I assume it is permitted to omit the `DIRS` and `APP_DIRS` configuration keys (with default values of `[]` and `False`, respectively) if a template engine is being configured to load templates only from a non-filesystem source? * Along a similar vein, I think it should be permissible (though not encouraged) for a template engine to refuse to define an app-dirs subdirectory name. In this case it would simply be an error to attempt to configure that template engine with `APP_DIRS: True`. * Is there any meaning to the top-level keys in the `TEMPLATES` setting? None is mentioned; the only one I can think of would be as a string that one could use in the option-1 API to explicitly select a specific engine. I think the ability to configure template engine priority will be important, and a dictionary is problematic for that. Importing OrderedDict is ugly boilerplate. So I wonder if `TEMPLATES` should be a list of dictionaries rather than a dictionary of dictionaries; perhaps with a `NAME` key if we need a string identifier for each configured engine? * Related to the above, I am glad to see that the proposed configuration scheme would allow configuring more than one instance of the same template engine. I don't have any use case for this off the top of my head, but my instinct says it's a useful capability to maintain. (This would imply that we shouldn't re-use the BACKEND string itself as an identifier for a particular configured engine.) * Re CSRF compatibility: by what convention does a template engine know if a `request` has been provided? (This is clarified by the template object interface code below, but is left unclear at the first mention of CSRF compatibility). * I'm not convinced that rejecting the "switch to Babel" option for makemessages actually reduces the scope / difficulty / disruptiveness of this DEP. I think improving makemessages to handle multiple template engines will likely involve an undesirable level of reimplementing things Babel already does well (whereas since Babel already has support for DTL syntax, makemessages' hacky handling of DTL could be removed with a switch to Babel). There is already precedent in Django for required dependencies for subsystems which are not enabled by default and do not block the initial getting-started flow. I think a more flexible makemessages will likely be best (and simplest) implemented as a wrapper to Babel, perhaps settings some configuration defaults according to what is known about the Django project structure. * Re Django backend: I think it should be an error to attempt to configure the DTL backend with `APP_DIRS: True` and `OPTIONS['LOADERS']` set. Allowing self-contradictory configurations to pass silently, ignoring one aspect of the user's expressed desires, is bad. * It's not mentioned explicitly, but I presume the DTL backend's `render` method will take an ordinary dict (not a `Context` instance) as its `context` argument, and will automatically use `RequestContext` (and thus context processors) if the `request` argument is provided? This seems like the best approach to me (though for backwards-compatibility it will also need to accept a `Context` instance, and even perhaps pull the request out of a `RequestContext` instance if provided, at least for a deprecation period). * That gets into the question of how the public API and implementation of `django.shortcuts.render` and `render_to_response` will change. This isn't fully outlined in the DEP; perhaps it should be, since for most people it's the primary entry point for templates? * I'm confused about how you plan to organize the Python namespaces for Django's template support. I see use of both `django.template` and `django.templates` in various code examples, but if there is a consistency to the usage I'm missing it (and I don't think differentiating modules by a single letter would be a good plan, anyway). Ideally it seems to me that the DTL (currently in `django.template` and must probably stay there for backwards-compatibility) and the new engine-configuration system would reside in totally separate namespaces; having the latter reside in a submodule of the former seems logically backward. The issue is finding a good name for the latter, since `django.template` would be the obvious choice. Perhaps `django.templating`? Or maybe your apparent choice of `django.template.backends` is the best option in practice, even if it implies a relationship that is inverted from the reality (`django.template` is one of the backends supported by `django.template.backends`; `django.template.backends` is not a feature of `django.template`). * Re jinja2 backend's 'env' option: can we remove the option to have that point directly to an instance, and require that it point to a callable accepting the other options and returning an instance? I don't see a benefit to this option that outweighs the additional complexity it introduces. In the worst case, if someone has a module-level instance already, it's a trivial one-liner (or two-liner if you don't like lambda) to create a function returning that instance. I don't agree with the assertion that `project_name.jinja2.env` will be the most convenient solution in general, because it means you have to manually handle the default values for `autoescape`, `loader`, `auto_reload`, and `undefined` that Django would otherwise provide. This is demonstrated by your example jinja2.py module, most of which is boilerplate which could be eliminated by instead defining a function that takes the options from settings, with defaults already handled. * You mention `SimpleTemplateResponse` and `TemplateResponse` in Appendix A and note that they aren't really features of DTL (which I agree with), but you don't provide anywhere a migration plan for them. I would like to see them usable with any template engine, which shouldn't be too hard. It does raise the question of whether they should be moved to a different location. * Re FAQ "Is it possible to use Django template filters or tags with other engines?" - I'm not sure I agree with the strong assertion that this "certainly will" be implemented as a third-party module. As you mention in the previous FAQ answer, in general it will be much less work and a better implementation for providers of custom DTL filters/tags to provide the core functionality via simple Python functions (which can then be trivially added to the context for most non-DTL template languages, including Jinja2), and the DTL integration as thin wrappers around the core function. Whew! Thanks again for all your work on this project, and for reading through all my comments :-) Carl -- 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/5457CDD5.8060206%40oddbird.net. For more options, visit https://groups.google.com/d/optout.
