[magic-removal] [patch] proposed update to template system

[django@…]

Django's template tags do not always resolve arguments consistently
and as more people write custom template tags there is a risk that
there will be a proliferation of arbitrary argument syntaxes, each
with its own quirks.

Currently, arguments are sometimes resolved as context variables
('if' tag and 'ifequal' in a slightly different way), sometimes as
pure tag-specific syntax ('as', 'in'), sometimes as Python
primitives (strings and numbers in 'simple_tag' tags but not strings
containing whitespace), sometimes variables take filters, sometimes
not... For people writing custom template tags it would be nice if
there was one (simple) way of doing things.

Some related tickets: #365, #566, #959, #1338

Robert's 'simple_tag' decorator introduced the nice idea of mapping
template tags directly to Python callables. The existing
implementation is a bit convoluted in order to allow the first
argument to be either an alternative name or the decorated function.
This seems a bit "magic":

register.tag(tag_function)
register.tag('another_name', tag_function)

How come the first argument changed its meaning?

register.inclusion_tag('mytemplate')(tag_function)

Why does inclusion_tag have a double-call syntax (if not used as a
decorator)?

Proposed solution

This patch to the magic-removal branch supersedes #1105 and provides a
more regular mapping from 'simple_tag' to python callables.
Argument resolution handles quoted strings with whitespace, boolean
literals, and optional named "keyword" arguments. Also fixes #1338.

This works as expected:

@register.simple_tag
def chant(message):
    return '<p>%s</p>' % message

{% chant "Django beats Chuck Norris." %}

outputs:

<p>Django beats Chuck Norris.</p>

Optional arguments are useful for letting template authors customise output:

@register.simple_tag(takes_context=True, takes_block=True)
def chant(context, block, color='red', bold=False, class_='chant'):
    attributes = {
        'style':'color:%s; font-weight:%s;' % (color, bold and 'bold' or 'normal'),
        'class':class_,
    }
    return '<p%s>%s</p>' % (
        ''.join(' %s="%s"' % (name, value) for name, value in attributes.items()),
        block.render(context),
    )

{% chant bold=True class="chuck-hidden chant" %}Django beats Chuck Norris.{% endchant %}

outputs:

<p style="color:red; font-weight:bold;" class="chuck-hidden chant">Django beats Chuck Norris.</p>

For a real-world use case, I've got a tag that
dynamically renders an arbitrary block using a truetype font with the
Python Imaging Library. Optional keyword arguments control color,
font, and size. See ​http://www.kieranholland.com/code/django/typeset-tag/

Patch summary:

1. Adds regular expression parser for template tag arguments that permits single or double quoted strings with whitespace and named 'keyword' arguments. If a keyword argument name is a Python keyword an underscore is appended.

2. Resolves literal True and False arguments to Python booleans.

3. TEMPLATE_STRING_IF_INVALID is not returned from resolve_variable - only at render time.

4. simple_tag decorator allows takes_context, and takes_block arguments (per #1105) and can either return a string to be inserted in the template or None.

5. The tag registration class has been simplified with the benefit that the inclusion_tag and simple_tag syntax is more consistent: the tag function always comes first and optional alternate names - the uncommon case - are provided by the 'name' keyword argument.

Summary of backwards-incompatible changes:

1. Alternative names for tag registration come after the tag function.

2. resolve_variable raises VariableDoesNotExist exception for any unresolved variable so the caller is responsible for handling this.

I have fixed all backwards-incompatible code that I could find in the
magic-removal branch, but there may be a few bits and pieces
remaining. All existing unit tests pass(*) and I've added a few for
the tag argument tokenizer and made some minor doc fixes.

• Except one I think should fail: In 'resolve_variable' if a variable is resolved to a callable and a TypeError is thrown because the callable expected arguments why should the exception be masked? Doesn't it indicate a templating error or am I missing something?

Thoughts?

Kieran

[django@…]

template patch

[Jacob]

I very much don't like the keyword-arguments part of this; template tags should be as simple as possible and accessible to non-programmers. I'm leaving this open since the rest of the patch seems like a pretty good idea, but this needs more thought.

[django@…]

Keyword arguments are optional and make certain things simpler for template authors. For example, in the case of my 'typeset' tag, linked above, it is way easier to give just the named arguments that you want to alter rather than to have to give the whole set of positional arguments to change just one default (how to remember the order of arguments?). I am all for keeping things simple and I do not want to see complex logic embedded in templates either, but I do not think that keyword arguments have this effect. There is a conceptual parallel for template authors with giving named attributes to HTML tags ( imagine if they were positional ;). The intention is to simplify customisation by template authors of features well-encapsulated in Python and add a degree of self-documentation to tag calls (which is one of the really nice things about keyword arguments in Python) rather than have every custom tag do its own arbitrary syntax.

[django@…]

@jacob if a html programmer can't understand keywords, he isn't a html programmer at all :)
keywords make certains things much easier then lists, where the template author has to remember the order instead of just using names which he most like can remember better, especially on complex tags
i'm +1 on this

[Antti Kaihola]

What about performance? I remember reading that the superior speed of "manual" tag argument parsing as it's done now was the main reason for not developing a more advanced parser.

Anyway, this looks very good to me, and since template tags are good for so many situations, it's definitely a plus if they're simple to create.

[django@…]

Simplistic testing suggests that rendering a template consisting of an 'upper' block tag (converts content to upper case) containing some plain text is about 20% slower with the simple_tag version. Given that custom tags are usually doing only a small part of the work in rendering a template this is unlikely to be significant. In the context of handling an entire request it is even less likely to be significant. The time saved in development _is_ significant in my experience. There is also no compulsion to use the feature. The built-in tags are written the standard way and that would remain an option for those who think it will improve performance.

[Antti Kaihola]

While waiting for this patch to be applied (or waiting in vain if it won't), I'd love to try it as a separate module instead of patching Django. Do you happen to already have made a cleanly separated library?

[django@…]

The easiest way of testing it (for you and me:) is to apply the patch to a spare copy of Django magic-removal branch. The way I do it is to keep a local branch with svk, but just a second checkout would do. I use a shell script to toggle PYTHONPATH between branches. Let me know if you have any problems - it'd be good to hear how you go.

[django@…]

django@…, do you have an uptodate patch. It doesn't apply to MR anymore.

[django@…]

Updated patch for template extension requirement

[django@…]

Patch updated for new template extension requirement.

[Russell Keith-Magee]

(In [3268]) Fixes #1338, Refs #1400, #2237 -- Modified variable resolution to allow template 'if' statements to work if TEMPLATE_STRING_IF_INVALID is set. Modified unit tests to force the use of this variable, so that returning isn't confused with an actual failure.

[Russell Keith-Magee]

(In [3269]) Refs #1400 -- Variable resolver now converts literal strings 'False' and 'True' into booleans when used as template arguments. This is point 2 from ticket #1400. Thanks Kieren Holland.

[Russell Keith-Magee]

Firstly, Kieran - apologies for the misspelled name on the last checkin.

Second - I'm going to mark this ticket wontfix - but that's just because it's a bit of a monster. I've cherry picked the easy and obvious bits (2 and 3), but as for the others:

1. I'm with Jacob, and not convinced of the efficacy of adding keyword arguments to the template language.

4. This is a duplicate of #1105 - discussion should continue there (where Robert made some points I am inclined to agree with)

5. Seems like a good idea, but the subset of the patch that relates to this change is no longer obvious.

If you want to pursue 1 and 5, open a new ticket that covers just that feature; pursue 4 in ticket #1105. Tickets should be atomic whenever possible. This makes tickets easier to understand, and more likely that they will be dealt with in a timely fashion.

[Luke Plant]

Kieran, Russell,

I only just noticed this (because of the django-updates not being sent out).

I'm -1 on the [3269] changeset. This is a backwards incompatible change (though not noted in the list of backwards incompatible changes), and it could introduce bugs into some of my existing code (and other people's I'm sure). Every time I have something like:

   {% if foo %}
      <p>{{ foo }}</p>
   {% endif %}

where foo is a string, I now have a bug in the case where foo happened to contain the string 'False'. I know it is unlikely, but now the code I have to write for correct behaviour is massively complicated.

When would you actually have a variable that contains the string 'False' instead of boolean False? The new behaviour is very unpythonic, and while I know that this is a template language, all the values being fed into the template system will be Python values, so I can't see any need for this.

[Simon Willison]

I agree with lukeplant. The 'True' => True thing is just asking for trouble.

[Russell Keith-Magee]

(In [3680]) Refs #1400 - Reverted r3269. Template variable evalution should follow Python norms.