django.template.TokenParser inconsistent when it comes to filters...

9 views
Skip to first unread message

Dmitri Fedortchenko

unread,
Nov 17, 2007, 9:27:53 PM11/17/07
to Django developers
The django.template.TokenParser has a little problem.

I am not sure if this is a problem actually, but it is inconsistent
when parsing filters that follow constant strings or variables.

Meaning that:
{% tag thevar|filter sometag %} will produce:
self.value() = "thevar|filter"
self.tag() = "sometag"

However:
{% tag "a value"|filter sometag %} will produce:
self.value() = "a value"
self.tag() = "|filter"
self.tag() = "sometag"

This does not seem like correct behaviour...
I made a very simple patch for this, thus the outcome of the above:

{% tag "a value"|filter sometag %} will produce:
value = "a value"|filter
tag = sometag

So now we can simply pass the "value" into a FilterExpression to parse
the filters...

Am I on the wrong track here?

Malcolm Tredinnick

unread,
Nov 17, 2007, 10:08:25 PM11/17/07
to django-d...@googlegroups.com

On Sat, 2007-11-17 at 18:27 -0800, Dmitri Fedortchenko wrote:
> The django.template.TokenParser has a little problem.
>
> I am not sure if this is a problem actually, but it is inconsistent
> when parsing filters that follow constant strings or variables.
>
> Meaning that:
> {% tag thevar|filter sometag %} will produce:
> self.value() = "thevar|filter"
> self.tag() = "sometag"
>
> However:
> {% tag "a value"|filter sometag %} will produce:
> self.value() = "a value"
> self.tag() = "|filter"
> self.tag() = "sometag"
>
> This does not seem like correct behaviour...
> I made a very simple patch for this, thus the outcome of the above:
>
> {% tag "a value"|filter sometag %} will produce:
> value = "a value"|filter
> tag = sometag

I think this is probably the right idea, yes. TokenParser is a bit of an
odd duck. Nothing in core uses it except the i18n template tags, so it
hasn't gotten as much attention during the recent Variable refactoring.
When I do work in the i18n template tags I realise that the API of
TokenParser really isn't particularly handy (it kind of cries out for an
iterator, for example). Fortunately, most changes can be made
incrementally and without backwards incompatibilities.

I've kind of been holding off on doing much work here since it currently
"mostly works" and Tom Tobin had some good ideas about making template
tag creation a bit easier. I've been hoping to work with him at some
point to get that polished up and into trunk. One of the places it would
help simplify is django.templatetags.i18n.

For now, I'd be happy if you'd create a ticket with your patch to make
the above change. It's slightly backwards incompatible, but not in a
hard-to-fix way for downstream developers (normally, the result of any
such expression is going to be passed to compile_filter() anyway and
this makes that easier).

Regards,
Malcolm

--
Remember that you are unique. Just like everyone else.
http://www.pointy-stick.com/blog/

Dmitri Fedortchenko

unread,
Nov 17, 2007, 10:55:00 PM11/17/07
to Django developers
Great.

Here are the tickets:
http://code.djangoproject.com/ticket/5971
http://code.djangoproject.com/ticket/5972

And while we're on the subject of tickets:
I've been digging around in the jungle of template parsing while
working on this patch and and the trans patch, and a thought occured
to me.
Why can't the Variable class handle processing of filters instead of
FilterExpression.resolve?

A Variable in a template is either a literal or a variable from the
context, and those can in most cases have filters applied to them.
Wouldn't it be nice if a Variable would simply have a "filters" list
and the resolve method would be the one doing the application of said
filters? (with the option to simply NOT apply the filters)

Obviously it's a pretty daring idea and I don't have much to back it
up right now, but my thought is that there seems to be way too many
steps involved in parsing a single token and there are many datatypes;
hard to keep track of everything.

If we look at it in the scope of tags and {{}} (what would I call
that?), then they can basically contain only a few datatypes:
literals, context variables, filters and parameters.

So why not make it a little more "defined" and give Variables the
power they deserve! ;)

It's 5am, so my thoughts are not coming out clearly. But if the above
makes you think "hmm, interesting", I can write up something more
detailed, maybe with some code examples...

//Dmitri

On Nov 18, 4:08 am, Malcolm Tredinnick <malc...@pointy-stick.com>
wrote:

Malcolm Tredinnick

unread,
Nov 17, 2007, 11:22:06 PM11/17/07
to django-d...@googlegroups.com

On Sat, 2007-11-17 at 19:55 -0800, Dmitri Fedortchenko wrote:
> Great.
>
> Here are the tickets:
> http://code.djangoproject.com/ticket/5971
> http://code.djangoproject.com/ticket/5972
>
> And while we're on the subject of tickets:
> I've been digging around in the jungle of template parsing while
> working on this patch and and the trans patch, and a thought occured
> to me.
> Why can't the Variable class handle processing of filters instead of
> FilterExpression.resolve?

Because Variable was introduced only very recently and
FilterExpression.resolve() has been there since about day one. The
incremental changes haven't got that far.


> A Variable in a template is either a literal or a variable from the
> context, and those can in most cases have filters applied to them.
> Wouldn't it be nice if a Variable would simply have a "filters" list
> and the resolve method would be the one doing the application of said
> filters? (with the option to simply NOT apply the filters)

It's that last point that points to why the current design might have
some benefits. At times we need to differentiate between the variable's
value itself and the variable after all filters have been applied.

There's still a possibility of folding things in together, but the
current version is completely silly either. It's mostly just a naming
issue (do you call the method on this object or that object?) ,though.
The functionality isn't likely to change too much.

> Obviously it's a pretty daring idea and I don't have much to back it
> up right now, but my thought is that there seems to be way too many
> steps involved in parsing a single token and there are many datatypes;
> hard to keep track of everything.
>
> If we look at it in the scope of tags and {{}} (what would I call
> that?), then they can basically contain only a few datatypes:
> literals, context variables, filters and parameters.
>
> So why not make it a little more "defined" and give Variables the
> power they deserve! ;)

I suspect you're really just talking about folding FilterExpression into
Variable, for the most part. Might be worth it eventually. The nice
thing is that this sort of stuff can all be done incrementally and
without too much external disruption. We don't want to go nuts and break
everybody's code needlessly, so any changes *must* bring very real
benefits and not just count as rearranging the deck chairs to make them
look nicer, because that's asking external filter writers to make
changes for no actual benefit.

Still, I agree with your thinking in general. Some of those internals
could do with a broader look now that we've got a couple years more
experience in how things are being used. We could encourage wider use of
TokenParser (and document it). We can incorporate Tom Tobin's ideas for
making it easier to write template tags. We can possibly tidy up some of
the internal interfaces (as you've no doubt seen,
django.templatetags.i18n does a lot of duplicate work that smells a bit
unnecessary and that's for historical reasons: more helper code has been
added since it was first written).

By all means, dive in and try some things out. Be very aware of
backwards compatibility. Or, rather, then pain inflicted when you break
it, so have a really good reason if you must do so. Try to work in
smallish incremental stages, as much as possible, so that our heads
don't explode when we try to review the patches.

Regards,
Malcolm

--
No one is listening until you make a mistake.
http://www.pointy-stick.com/blog/

Reply all
Reply to author
Forward
0 new messages