Code

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1400 closed enhancement (wontfix)

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

Reported by: django@… Owned by: russellm
Component: Template system Version:
Severity: normal Keywords:
Cc: django@… Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

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.
  1. Resolves literal True and False arguments to Python booleans.
  1. TEMPLATE_STRING_IF_INVALID is not returned from resolve_variable - only at render time.
  1. 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.
  1. 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.
  1. 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

Attachments (2)

templates_r2388.diff (37.7 KB) - added by django@… 8 years ago.
template patch
templates_r2738.diff (38.1 KB) - added by django@… 8 years ago.
Updated patch for template extension requirement

Download all attachments as: .zip

Change History (18)

Changed 8 years ago by django@…

template patch

comment:1 Changed 8 years ago by 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.

comment:2 Changed 8 years ago by 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.

comment:3 Changed 8 years ago by 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

comment:4 Changed 8 years ago by akaihola

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.

comment:5 Changed 8 years ago by 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.

comment:6 Changed 8 years ago by akaihola

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?

comment:7 Changed 8 years ago by 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.

comment:8 Changed 8 years ago by django@…

  • Cc django@… added

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

Changed 8 years ago by django@…

Updated patch for template extension requirement

comment:9 Changed 8 years ago by django@…

Patch updated for new template extension requirement.

comment:10 Changed 8 years ago by russellm

  • Owner changed from adrian to russellm

comment:11 Changed 8 years ago by russellm

(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.

comment:12 Changed 8 years ago by russellm

(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.

comment:13 Changed 8 years ago by russellm

  • Resolution set to wontfix
  • Status changed from new to closed

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.
  1. This is a duplicate of #1105 - discussion should continue there (where Robert made some points I am inclined to agree with)
  1. 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.

comment:14 Changed 8 years ago by lukeplant

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.

comment:15 Changed 8 years ago by Simon Willison

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

comment:16 Changed 8 years ago by russellm

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.