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 django-developers+***@googlegroups.com.
To post to this group, send email to django-***@googlegroups.com.
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.