[WIP] force jinja to preserve native types #23943
[WIP] force jinja to preserve native types #23943jctanner wants to merge 11 commits intoansible:develfrom jctanner:test_jinja_hack
Conversation
There was a problem hiding this comment.
s/stry,bytes,unicode/string_types,text_types/
There was a problem hiding this comment.
this is not compatible across python versions use text_strings, byte_strings from our 'compatiblity' code instead
There was a problem hiding this comment.
Camel case and underscores? :)
There was a problem hiding this comment.
sometimes you have to allow for all kinds ...
| elif len(invals) > 1: | ||
| # cast to unicode and join | ||
| invals = u''.join([u'%s' % x for x in invals]) | ||
| return invals |
There was a problem hiding this comment.
We could make this more efficient. what types can be given here? Since the original jinja2 code is u"".join(invals) it seems like the minimum assumptions we can make are that invals is an iterable and that all the elements can convert to a text type without unicode exceptions. So maybe something like this:
# We can omit this block if invals is never a string type
if isinstance(invals, six.text_type):
return invals
elif isinstance(invals, six.binary_type):
return to_text(invals, errors='surrogate_or_strict')
try:
if len(invals) == 1:
# Break out the single value
return invals[0]
except TypeError:
# Iterable (by contract) but not a Sequence
pass
return u''.join(invals)- This keeps us from instantiating two lists.
- This should be faster even than the jinja2 concat if strings (not inside of a container) are allowed as invals.
- If we don't have to deal with strings, then we can get rid of that block and reduce the number of conditionals.
I can adapt that further if we need to make less assumptions (or if we can make more of them).
There was a problem hiding this comment.
Okay, jctanner says that this is always a generator. So this is a better implementation:
from itertools import chain
start = []
for val in invals:
start.append(val)
if len(start) > 1:
break
else:
if start:
return start[0]
return []
return u''.join(chain(start, invals))There was a problem hiding this comment.
Profiling and jctanner's version is faster. So the only things I can see to do differently:
- remove "if isinstance(invals, list):". after the list comprehension, invals should always be a list.
- u'%s' % x for x in invals note that this isn't 100% safe (for instance b'café' will raise a unicode error) but I assume you took it from some part of jinja2 code so we aren't making a new assumption then.
|
Another usecase that came up on IRC today ... A and B will preserve their bool types in that case with this patch and no downstream consumers would have to guess at the intent. |
|
another example of same boolean: vars:
A: '{{ a_node is defined }}'
B: '{{ b_node is undefined }}' |
|
I'm happy to see that the upstream changes have landed and will be in 2.10. 👍 Any chance this will make it in to Ansible before then to make this feature available when using, say, Jinja 2.9? Or is the plan to wait until 2.10 gets released, and use that? |
|
@cognifloyd, I commented in #ansible-devel with much the same query, and jtanner responded with |
|
Sorry about mis-posting this message moments ago. Jinja 2.10 is out. What is involved in increasing ansible's version requirements? Is that something that can take a year or two or more? What makes the release "mainstream"? RPM releases in RHEL or packages in other distros or ...? |
|
New PR #32738 |
Subclasses the CodeGenerator class and refactors visit_Output to exclude the default to_string() class it injects.
SUMMARY
Subclasses the CodeGenerator class and refactors visit_Output to exclude the default to_string() class it injects.
Upstream effort: pallets/jinja#708
ISSUE TYPE
COMPONENT NAME
templar
ANSIBLE VERSION