Hi Aymeric,
Thanks for the feedback!
>
Even if it isn’t consistently implemented, the loader API is documented:
>
> https://docs.djangoproject.com/en/1.7/ref/templates/api/#using-an-alternative-template-language
> To sum up:
> load_template_source(template_name) —> (template_string,
> template_origin)
> load_template(template_name) —> (template, template_origin)
> The behavior is loosely defined; anything that matches the signature
> is fine.
> template_origin is especially ill-defined.
> Are you proposing to change the API outright? Or are you planning to
> implement a deprecation path for existing third-party template loaders?
The deprecation is gradual. Old loaders continue to work but can't take
advantage
of recursive extending until they implement get_template_sources and
get_internal.
How would a loader that loads templates from a database implement this
> method?
> It seems to me that the proper way to define such a method is “yields
> some
> opaque objects that implement the following API: …” I suspect that API
> would be quite similar to the Origin API — maybe an idealized version
> if
> the current version doesn’t suffice.
My original thought was it would yield a model instance which is then
passed to
get_internal. Your right, though. It'd be better to return an origin or
other opaque object. This is a nice change. I'll update the loaders so they
have a consistent return value.
Attaching this method to the objects yielded by get_template_sources
> would
> provide better encapsulation than attaching it to the Loader itself.
Agreed.
At first sight adding a “skip” optional argument to Engine.get_template,
> Engine.find_template, BaseLoader.__call__ and BaseLoader.load_template
> may work. Could you explain where the difficulty lies?
There are a few reasons I chose to go with a new api:
First, the advantage of "get_template" over "load_template" is that it calls
"get_template_sources" directly. This means the skip and debug information
is
readily available in a single place.
If "skip" is added to load_template instead, it also needs to be added to
"load_template_source". That means the skip and debug logic needs to be
duplicated in each of the loaders, including third-party ones. This is
because
"load_template_source" is where the looping through template sources
currently
exists.
Second, the cached loader reimplements most of load_template with additional
logic. If "get_template_sources" can be counted on, this cleans it up quite
a
bit.
Third, returning (template, origin) is redundant if origin is a property of
template. If we change load_template, that ends up sticking around, unless
we later introduce another deprecation for it.
With that said, it's possible to refactor "load_template" to stop using
"load_template_source" and instead call "get_template_sources". If it does
this I can address 1 and 2. I'm not sure if we can do 3. We'd still need to
do the following:
* Add a deprecation period because get_template_sources is required to
support
template recursion.
* If get_template_sources is used, there still needs to be a get_internal
method of some sort. I could try to reuse load_template_source, but it's
a pretty big change in behavior of what that function currently does. I'm
not sure this lends itself to a nice upgrade path.
Therefore, I see the deprecation period for 3rd-party loaders being one of:
1. Add get_template_sources and get_internal
2. Add get_template_sources and modify load_template_source
Do you still prefer refactoring load_template instead? If so, should I add
get_internal or try to modify load_template_source?
Your design makes sense. My main suggestion is to fit it in the current
> APIs.
> I would try to reuse load_template instead of introducing get_template
> and to
> hook to a revamped Origin API. The Origin API was documented in Django
> 1.7; changing it it less disruptive than changing the Loader API. Did
> you try
> this, and if you did, why didn’t it work?
Okay. I think this is how the api would look then:
get_template_sources(template_name) -> yields origin(s)
get_internal(origin) -> template_string
get_template(template_name) -> template
--or--
load_template(template_name) -> (template, template_origin)
This API is still tied to the Django Template Language. I would prefer
> a generic
> list of (“template identifier”, “loading mechanism”, “reason for
> failure”) that every
> backend could implement.
Yes, this is an easy change. I'll add it into my branch.
Furthermore, I’m not sure what this list represents. Is it a “template
> call stack”
> i.e. the chain of templates included up to the point where loading a
> template
> failed? Or is it a list of ways the template backend tried to load the
> failing
> template before giving up?
Sorry. I didn't explain that example clearly, and it was slightly wrong. If
by
"template call stack" you mean the complete list of sources that were
tried, then
yes. This means that for a "get_template" call, it's simply a linear list
of all tried
sources. For extending, it includes information about the initial
get_template
call, then all the subsequent calls in the extends node.
For instance, if you had a filesystem loader with two folders that extended
a
missing "base.html", tried would appear as:
[
(engine, "/path/to/fs/index.html", "filesystem.Loader", "Found")
(engine, "/path/to/fs/base.html", "filesystem.Loader", "Source does not
exist")
(engine, "/path/to/fs2/base.html", "filesystem.Loader", "Source does
not exist")
]
Assume the same example found "base.html" in fs and it also extended
"base.html". If "base.html" didn't exist in fs2, tried would appear as:
[
(engine, "/path/to/fs/index.html", "filesystem.Loader", "Found")
(engine, "/path/to/fs/base.html", "filesystem.Loader", "Found")
(engine, "/path/to/fs/base.html", "filesystem.Loader", "Skipped")
(engine, "/path/to/fs2/base.html", "filesystem.Loader", "Source does
not exist")
]
Again, if neither base.html or index.html exist in fs, and fs2 has
index.html but not
base.html:
[
(engine, "/path/to/fs/index.html", "filesystem.Loader", "Source does
not exist")
(engine, "/path/to/fs2/index.html", "filesystem.Loader", "Found")
(engine, "/path/to/fs/base.html", "filesystem.Loader", "Source does not
exist")
(engine, "/path/to/fs2/base.html", "filesystem.Loader", "Source does
not exist")
]
Perhaps it'd be helpful to annotate "Found" as "Extended" when extending.
Did that explain it better? If not, I can try again. :)
I agree with making it always available. The real question is about the
> API itself.
> We documented the existing concrete implementations in Django 1.7 but
> they’re
> inconsistent and their purpose is still unclear.
https://docs.djangoproject.com/en/1.7/ref/templates/api/#
> template-origin
Oops, that was my fault. :/
I was looking for something easy to contribute to:
https://code.djangoproject.com/ticket/16096
In the pull request discussion Tim was wary about removing the debug check
for
adding the origin. The use-case for the ticket was to display the current
template path to front-end devs. I'm not sure it has seen much usage in the
wild.
I was mainly thinking about providing tracebacks for errors in
> templates. Jinja2
> has fancy integration with Python’s traceback system. Django emulates
> it in its
> debug view with exc.django_template_source and
> get_template_exception_info.
> That’s independent from template loading exceptions.
Okay. Great. I see your branch has been merged. I'll start work on getting
mine
updated with your suggestions so far.
Thanks!
Preston
--
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/2da1e089-2b57-4a43-b198-5eb942b976c3%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
