[Django] #21065: Internally choosing how to process a template is inconsistent
Django
-----------------------------------------+--------------------
Reporter: Keryn Knight <django@…> | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.5
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------------+--------------------
[https://github.com/django/django/tree/ec47de77d6a7bc9327047d157fb5706751413f18/django/template/base.py#L1201
inclusion_tag] checks the following:
* Is it a Template instance?
* Or is it ''not'' a string, and ''is it iterable?'' '''is_iterable would
allow genexps/dictionaries through'''
* Both the above failed? Should be a string.
[https://github.com/django/django/tree/ec47de77d6a7bc9327047d157fb5706751413f18/django/template/loader.py#L159
render_to_string] checks the following:
* is it a ''list or tuple''? '''doesn't allow genexps/dictionaries/etc'''
* No? It must be a string
* '''doesn't account for being passed a Template instance at all'''
[https://github.com/django/django/tree/ec47de77d6a7bc9327047d157fb5706751413f18/django/template/response.py#L53
SimpleTemplateResponse] checks the following:
* is it a ''list or a tuple'' ('''consistent with render_to_string''')
* No; is it a string? ('''consistent with render_to_string''')
* No; ok, it must be a template instance ('''inconsistent with
render_to_string, sort of consistent with inclusion_tag''')
I discovered this while trying to choose how to fix #20995, which would
again be different; I don't really want to push a pull request for that
one without knowing what the outcome of this is, though. It might be worth
introducing a single function (`resolve_template` or something) which
provides a consistent way to take an input and decide how to get from it
to a Template instance.
--
Ticket URL: <https://code.djangoproject.com/ticket/21065>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Keryn Knight | Owner: nobody
<django@…> | Status: new
Type: | Version: master
Cleanup/optimization | Resolution:
Component: Template system | Triage Stage:
Severity: Normal | Unreviewed
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by Keryn Knight <django@…>):
* needs_better_patch: => 0
* component: Uncategorized => Template system
* needs_tests: => 0
* version: 1.5 => master
* needs_docs: => 0
* type: Uncategorized => Cleanup/optimization
--
Ticket URL: <https://code.djangoproject.com/ticket/21065#comment:1>
Django
Reporter: Keryn Knight | Owner: nobody
<django@…> | Status: new
Type: | Version: master
Cleanup/optimization | Resolution:
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by mjtamlyn):
* stage: Unreviewed => Accepted
Comment:
+1 to improving the consistency here. I think it's safe from a compat
point of view because the three cases should be independent of each other.
I like the idea of a proper function with public api for doing this, it
seems common for someone wishing to implement extensions or alternatives
to the API without worrying about this exact implementation.
--
Ticket URL: <https://code.djangoproject.com/ticket/21065#comment:2>
Django
Reporter: Keryn Knight | Owner: nobody
<django@…> | Status: new
Type: | Version: master
Cleanup/optimization | Resolution:
Component: Template system | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Keryn Knight <django@…>):
I've thought about this some, and so far I'd be inclined to introduce a
function, which I'm referring to as `resolve_template`; the idea being to
turn any given input into something which ultimately returns either a
`Template` instance, or raises `TemplateDoesNotExist`. What I've come up
with though, does mostly step on the toes of `select_template`:
* `resolve_template` would take '''one''' argument;
* if the argument ''isinstance'' of six.string_type, or `Template`, turn
it into a tuple of that (allowing us to avoid needing to do the
insinstance shuffle as much further in):
{{{
input = 'mystring'
if insinstance(input, six.string_type):
input = (input,)
}}}
* otherwise, consider the argument to be an iterable of some sort.
* for each item in the iterable:
* if it's a `Template` instance, return it
* if it's a string, use `get_template`, returning if it resolves to a
`Template`, or continuing if it raised `TemplateDoesNotExist`
* If the iterable is exhausted without finding anything, raise
`TemplateDoesNotExist`
It's ''pretty much'' `select_template`, though I'm not familiar enough
with the loaders behind the scenes to say whether or not there's any
idiosyncrasies which would preclude the changes from being applied
directly to `select_template`, especially WRT to the guarantee of
backwards compatibility.
The larger point however, is that it should be entirely feasible for the
following use cases, which aren't guaranteed right now because of the
multiple implementations:
* I want to provide a list of template strings for the template loader to
resolve, with a `Template` instance at the end (or short circuited into
index '''N''' if I so desire):
{{{
['app/model.html', 'app/fallback.html', Template('{{ x }}')]
}}}
* I want to provide a genexp of templates to be resolved, because why not:
{{{
things = (x for x in y)
}}}
* I have a `deque` object already, and I want to pass it directly to the
template engine:
{{{
things = deque(['app/model.html', 'something/else.html'])
}}}
I have an `OrderedDict`/`SortedDict` whose keys map to template loader
locations, and whose values I'm already making use of elsewhere:
{{{
a = OrderedDict((('app/model.html', 1), ('app/model2.html', 2)))
}}}
The only 'problem' with providing this kind of flexibility is less in the
implementation (I hope) than the fact that some iterables simply wouldn't
be suited for template resolution, namely: `dict`, `set` and other
inherently unordered objects/types. Its impossible to reasonably accept
those as input, because Django can no longer guarantee order of template
resolution. This kind of worry is, I expect, what led to the whitelist of
iterable types for the most part, rather than a blacklist, though as the
original ticket indicates, `inclusion_tag` might allow those through by
accident at the moment.
Ultimately, this could then be used to replace many uses of `get_template`
and `select_template` and the associated sniffing of types in areas
outside of the template package, where it arguably belongs as an
implementation detail.
--
Ticket URL: <https://code.djangoproject.com/ticket/21065#comment:3>
Django
Reporter: Keryn Knight | Owner: nobody
<django@…> | Status: new
Type: | Version: master
Cleanup/optimization | Resolution:
Component: Template system | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: berker.peksag@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/21065#comment:4>
Django
Reporter: Keryn Knight | Owner: nobody
<django@…> |
Cleanup/optimization |
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
The landscape changed when I merged the multiple template engines branch.
Some parts may be more consistent now.
--
Ticket URL: <https://code.djangoproject.com/ticket/21065#comment:5>
Django
Reporter: Keryn Knight | Owner: nobody
<django@…> |
Type: | Status: new
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bcail):
* cc: bcail (added)
Comment:
One option would be to add a `resolve_template()` method/function to the
engine and loader modules. The `resolve_template()` function could handle
choosing between `select_template()` and `get_template()`.
This would allow removing `resolve_template()` in
`SimpleTemplateResponse`, and `Engine.render_to_string` and
`InclusionNode` could use the `resolve_template()` in the `engine` module.
If there's interest in this, I could open a PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/21065#comment:6>