Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#7799 closed (wontfix)

django.template refactoring

Reported by: emulbreh Owned by: nobody
Component: Template system Version: master
Severity: Keywords: tplrf
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: UI/UX:

Description

Why should django.template be refactored?

Filter and Variable parsing is inconsistent. Many ad hoc parsers in defaulttags are fragile.
Whitespace handling is ungraceful.

The patch provided splits __init__.py into separate modules and introduces a TokenStream
class that allows parsing of literals, vars (called lookups) and filter expressions. No docs yet,
have a look at the source (it all happens in ~150 loc).

Additionally some features crept in, listed below.
The patch only touches django.template and django.templatetags.
Tests are separate so you can run them with the old code.

Modules in django.template

  • context (no dependencies)
    Mostly untouched. There's a Context level template cache included, accessible via Context.get_template() and Context.select_template(). This is used in IncludeNode, ExtendsNode, and inclusion_tag. Slightly backwards incompatible if you rely on parse time sideeffects (ie {% load %}) in included templates.


A loader kwarg is added to Context to force use of a specific templateloader (something with a get_template() method).

class PrefixLoader:
    def __init__(self, prefix_list):
        self.prefix_list = prefix_list
    def get_template(name):
        from django.template.loader import select_template
        return select_template(['%s/%s' % (prefix, name) for prefix in self.prefix_list])

tpl.render(Context({}, loader=PrefixLoader(['a', 'b', 'c'])))


  • expressions (no dependencies)
    Contains Lookup, Literal, and FilterExpression. All are subclasses of a marker class Expression. These replace Variable and the old FilterExpression class. A Variable equivalent is provided in compat.Variable.


  • nodes (depends on expressions)
    Contains Node, NodeList, TextNode, and ExpressionNode. The latter is the old VariableNode renamed. A VariableNode alias is provided in compat.


  • compiler (depends on context, expressions, nodes)
    Contains Parser, Lexer, Token, TemplateSyntaxError, Origin, StringOrgin, and compile_string(). Those are mostly untouched. Additionally there are TokenStream, and TokenSyntaxError. Those provide a safe way to parse Tokens containing expressions.


  • library (depends on compiler, nodes, context)
    Contains InvalidTemplateLibrary, Library, get_library(), and add_to_builtins(). Mostly untouched.


  • loader
    Untouched. Moved TemplateDoesNotExist here.


  • utils
    ConditionalNode, base class for IfNode, IfEqualNode. EmptyNode. Helper functions: parse_conditional_nodelists(parser, name), parse_as(bits) parse_context_map(bits), render_with_context_map(renderable, context_map, context). parse_args_and_kwargs(bits), resolve_args_and_kwargs(args, kwargs)



  • compat
    Provides backwards compatibility for Variable, VariableNode, TokenParser, and resolve_variable()


  • defaulttags, loader_tags
    Refactored to use TokenStream where appropriate. Extended with-tag syntax to allow
    {% with foo as x, bar as y, ... %}
    
    Extended include-tag syntax to allow
    {% include "template" with foo as x, bar as y %}
    



Tickets

  • #1105 - takes_context and takes_nodelist for simple_tag
    fixed, based on julien's patch. tests not included to keep the tplrf-tests.diff functional for current trunk.
  • #2949 - templating engine too tightly tied to TEMPLATE_DIRS in the settings fixed through Context(loader=...).


  • #3544 - recursive includes
    fixed. test included.


  • #4278 - get_template should accept a dirs argument
    fixed through Context(loader=...).


  • #4746 - allow whitespace before and after filter separator
    fixed. tests are there (filter-syntax03, filter-syntax04) but expect TemplateSyntaxError.


  • #5270 - empty string literals
    fixed. tests included.


  • #5756 - accept filters (almost) everywhere
    fixed. a few tests included.



  • #5971 - token parser bug
    TokenParser will be deprecated.


  • #6271 - filter arguments with spaces
    fixed. test included. (seems to be fixed in trunk already)


  • #6296 - expressions for ifequal
    fixed. tests included (dup of #5756 ?)


  • #6510 - get_nodes_by_type() problem probably fixed.


  • #6535 - negative numeric literals
    fixed. tests included.


  • #6834 - support for templates to be loaded from dynamically-selected directories
    fixed through Context(loader=...)


  • #7295 - quotes, escaping and translation of string literals handled inconsistently in templates
    fixed.


  • #7438 keyword support for simple_tag and inclusion_tag
    not fixed yet. but utils.parse_args_and_kwargs() would make this easy. it's there for {% url %} anyway.


Performance

Comparing runtests.py templates measures, it appears the refactored version is faster.
But I'm not sure why and don't really trust these numbers. Are failing tests expensive?

Todo

{% if not %} is currently valid. The refactored implementation is greedy and reports a syntax error.
Could be fixed, not sure it's worth it. Broken tests are: if-tag-not02, if-tag-not03. DDN.

Numeric literals ending with "." are now considered a syntax error.
Broken test: ifequal-numeric07. DDN.

{% url %} currently accepts an arbitrary unquoted unicode string for the view name.
The refactored implementation only accepts bare python identifiers or quoted string literals.
Broken test: url05. DDN.

TemplateSyntaxError messages need some work. Eventually shortcut methods on TokenStream.

Find better names for TokenSyntaxError and TokenStream.

Provide a decorator that handles the boilerplate bits=parser.token_stream(token)
and bits.assert_consumed() calls.

Docs and more tests.

#1199 - multiple filter arguments
I'd like to implement this. But the proposed syntax foo|bar:x,y,z will not work
for {% url %} and the new {% with %}.
Proposals:

  • New syntax: foo|bar(x,y,z), keeping foo|bar:x for bc.
  • Tupel primitives: foo|bar:(x,y,z), would allow {% for x in (a,b,c) %}
  • Lisp style: foo|bar:(x|cons:y), this would basically allows bracketed expressions as arguments to filters. Arguably the ugliest.


#7261 - __html__() support
This could be useful. But would be easy to abuse.

Attachments (2)

tplrf.diff (115.7 KB) - added by emulbreh 7 years ago.
tplrf-tests.diff (8.6 KB) - added by emulbreh 7 years ago.

Download all attachments as: .zip

Change History (9)

Changed 7 years ago by emulbreh

Changed 7 years ago by emulbreh

comment:1 Changed 7 years ago by emulbreh

  • Has patch set
  • Keywords tplrf added
  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set

Note: #1199 would allow to deprecate _() magic and replace it with regular filters: {% foo|bar:(myvar|gettext) %}. Of course you could also introduce the concept of template functions, but if I wanted jinja, I'd use jinja.

comment:2 Changed 7 years ago by mtredinnick

This looks like a very large ticket and it's probably a bit risky to have just created it, rather than as a result of a django-dev discussion (since people might be tempted to put design thinking into the ticket comments). That being said, you should always feel free to work on whatever you like.

However (and this is very important), if you're refactoring the template stuff, then that is all you should be doing in this patch. No new features! Combining large changes that are only meant to reorganise the code (and not cause regressions) with things that change features isn't fair to anybody and will lead to the ticket being closed as wontfix because you aren't doing things in a way that allows us to work properly. A number of the changes you are proposing are reasonably controversial and bear in mind that Tom Tobin (korpios) is also working in this are on some refactoring (which he announced on django-dev), so you should probably check in with him, too.

So, yes, there's probably some stuff to do here, but keep things manageable. Make it just a refactoring. Those other tickets exist to solve their individual problems already and one issue per ticket is the preferred way to work unless it's absolutely unavoidable (we never mind putting certain fixes on hold until a reasonably certain refactor lands, so that isn't an issue if the refactoring is sensible). Separate out things that are actually problematic from things where you would just prefer a different name -- which usually are just rearranging the furniture rather than improvements, since everybody likes different names, although there are exceptions of course -- and from the things that add new functionality.

comment:3 Changed 7 years ago by julien

How about a template-refactor branch? Considering the large number of tickets involved, having a separate branch of Django may be easier to test and work with...

comment:4 Changed 7 years ago by mtredinnick

No justification for it at the moment. All the refactoring I've seen suggested here and in django-dev threads are small self-contained changes. And all the tickets are self-contained, too. Branches are complex to manage.

comment:5 Changed 7 years ago by emulbreh

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

django-dev seems like a bad place to discuss anything unrelated to 1.0.
That's why I did not even try this time. I actually wanted people to put
design thinking into the ticket comments, that could be summarized and
posted to django-dev in a month .. or five.

I agree new features should be separate tickets. I was just being lazy. So I'll
close this now and open a new ticket with a stripped down patch, since I cannot
change the ticket description (can I?).

And avoid new branches at all costs, please.

comment:6 Changed 7 years ago by adrian

At first glance, I like this a lot. Yes, please open a new ticket with a slimmed-down patch.

comment:7 Changed 7 years ago by emulbreh

New ticket here: #7806.

Note: See TracTickets for help on using tickets.
Back to Top