Opened 11 years ago
Last modified 9 months ago
#21065 new Cleanup/optimization
Internally choosing how to process a template is inconsistent
Reported by: | Owned by: | nobody | |
---|---|---|---|
Component: | Template system | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | berker.peksag@…, bcail | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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.
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
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.
Change History (6)
comment:1 by , 11 years ago
Component: | Uncategorized → Template system |
---|---|
Type: | Uncategorized → Cleanup/optimization |
Version: | 1.5 → master |
comment:2 by , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 11 years ago
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 aTemplate
, or continuing if it raisedTemplateDoesNotExist
- if it's a
- 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.
comment:4 by , 11 years ago
Cc: | added |
---|
comment:5 by , 10 years ago
The landscape changed when I merged the multiple template engines branch. Some parts may be more consistent now.
comment:6 by , 9 months ago
Cc: | added |
---|
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.
+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.