smart if tag

37 views
Skip to first unread message

Luke Plant

unread,
Nov 28, 2009, 12:40:54 PM11/28/09
to django-d...@googlegroups.com
Hi all,

I started work replacing Django's if with the "smart-if" template tag
by Chris Beaven ( http://www.djangosnippets.org/snippets/1350/ )

Of course, it is not as simple as it looks...

4 issues:

1) Handling non-existent variables
==================================

The current smart-if does not handle the case of non-existent
variables like Django does. (For example, something like

{% if foo or bar %}

when one of foo or bar is not defined in the context.)

To do this properly, I think the best approach would be something like
"almost three valued logic", which would work as follows:

1) variables that don't exist are represented by Null
2) all operators return either True or False, never Null
3) for 'not', 'and' and 'or', Null effectively acts like False
(so "not Null" == True, "Null or True" == True etc)
4) for all comparison operators, if either argument is Null then
the result is False e.g. "Null == Null" is False
"Null > 1" is False, "Null <= 1" is False, except
"!=", which should return true if either value is Null.

Any comments on this approach? I've started to implement this, and it
works OK so far, just needs a bit more work.

2) TEMPLATE_STRING_IF_INVALID breaks everything
===============================================

The smart 'if' tag allows for filters:

{% if articles|length >= 5 %}

To achieve this, it puts its arguments through FilterExpression. If
settings.TEMPLATE_STRING_IS_INVALID is not an empty string, this means
that:

{% if foo %}blah{% endif %}

will render 'blah' instead of '' if 'foo' is not in the context. This
causes test failures at the moment. We could change the tests, but
that is a backwards incompatibility, and it highlights a fundamental
change in the behaviour of {% if %}, which basically makes
TEMPLATE_STRING_IF_INVALID useless for even its stated purpose of
debugging, because the logic of a template will now be changed.

Personally, I care nothing for TEMPLATE_STRING_IF_INVALID, as there
are other things that it breaks, and I never use it. But obviously I
can't just think of myself here.

If there was an easy solution that didn't change the behaviour of the
if tag in this case that would be great, but I can't see one.

3) Behaviour of 'x and b or y'
==============================

Previously this was a syntax error. In Chris's code, it is fine and
works like Python. I would vote for keeping the smart-if as it is, as
I don't see the value of complicating the implementation to disallow
something that could be useful sometimes.

4) Behaviour of 'not'
=====================
Current Django will interpret 'not' as a variable if there is no
ambiguity e.g.

{% if not %}blah{% endif %}

This is not documented, but the behaviour is specified in detail in
the tests, so changing it would be a backwards incompatibility. The
smart-if is different (the above is a syntax error), and I suspect
that making the smart-if behave like current Django could complicate
things a lot.

IMO, current Django behaviour here is complete misfeature! I don't
know of any other language where keywords can be treated as variables
if the keyword doesn't make sense in that position...

Regards,

Luke

--
"He knows the way I take: when he has tried me, I shall come forth
as gold" (Job 23:10).

Luke Plant || http://lukeplant.me.uk/

Alex Gaynor

unread,
Nov 28, 2009, 7:43:03 PM11/28/09
to django-d...@googlegroups.com
On Sat, Nov 28, 2009 at 11:40 AM, Luke Plant <L.Pla...@cantab.net> wrote:
> Hi all,
>
> I started work replacing Django's if with the "smart-if" template tag
> by Chris Beaven ( http://www.djangosnippets.org/snippets/1350/ )
>
> Of course, it is not as simple as it looks...
>
> 4 issues:
>
> 1) Handling non-existent variables
> ==================================
>
> The current smart-if does not handle the case of non-existent
> variables like Django does.  (For example, something like
>
>  {% if foo or bar %}
>
> when one of foo or bar is not defined in the context.)
>
> To do this properly, I think the best approach would be something like
> "almost three valued logic", which would work as follows:
>
> 1) variables that don't exist are represented by Null
> 2) all operators return either True or False, never Null
> 3) for 'not', 'and' and 'or', Null effectively acts like False
>   (so "not Null" == True, "Null or True" == True etc)
> 4) for all comparison operators, if either argument is Null then
>   the result is False e.g. "Null == Null" is False
>   "Null > 1" is False, "Null <= 1" is False, except
>   "!=", which should return true if either value is Null.
>
> Any comments on this approach? I've started to implement this, and it
> works OK so far, just needs a bit more work.
>

Wouldn't that be a break from how {% ifequal %} works? Unless I'm
mistaken if you have a nonexistant value there it becomes None, which
is equal to None.

> 2) TEMPLATE_STRING_IF_INVALID breaks everything
> ===============================================
>
> The smart 'if' tag allows for filters:
>
> {% if articles|length >= 5 %}
>
> To achieve this, it puts its arguments through FilterExpression.  If
> settings.TEMPLATE_STRING_IS_INVALID is not an empty string, this means
> that:
>
> {% if foo %}blah{% endif %}
>
> will render 'blah' instead of '' if 'foo' is not in the context.  This
> causes test failures at the moment.  We could change the tests, but
> that is a backwards incompatibility, and it highlights a fundamental
> change in the behaviour of {% if %}, which basically makes
> TEMPLATE_STRING_IF_INVALID useless for even its stated purpose of
> debugging, because the logic of a template will now be changed.
>

I'm almost positive Malcolm did some refactoring in this area that
lets you get something other than TEMPLATE_STRING_IF_INVALID for an
invlaid FilterExpression.

> Personally, I care nothing for TEMPLATE_STRING_IF_INVALID, as there
> are other things that it breaks, and I never use it. But obviously I
> can't just think of myself here.
>
> If there was an easy solution that didn't change the behaviour of the
> if tag in this case that would be great, but I can't see one.
>
> 3) Behaviour of 'x and b or y'
> ==============================
>
> Previously this was a syntax error.  In Chris's code, it is fine and
> works like Python. I would vote for keeping the smart-if as it is, as
> I don't see the value of complicating the implementation to disallow
> something that could be useful sometimes.
>

+1 on allowing that.

> 4) Behaviour of 'not'
> =====================
> Current Django will interpret 'not' as a variable if there is no
> ambiguity e.g.
>
>   {% if not %}blah{% endif %}
>
> This is not documented, but the behaviour is specified in detail in
> the tests, so changing it would be a backwards incompatibility. The
> smart-if is different (the above is a syntax error), and I suspect
> that making the smart-if behave like current Django could complicate
> things a lot.
>
> IMO, current Django behaviour here is complete misfeature! I don't
> know of any other language where keywords can be treated as variables
> if the keyword doesn't make sense in that position...
>

It's kind of wacky behavior, agreed. But if should be sort of trivial
to implement right? Just if len(bits) == 1 and bits[0] == "not". Or
is this expected to work in cases like "if a and not". If the latter
is expected to work than it'll be a pain I think.

> Regards,
>
> Luke
>
> --
> "He knows the way I take: when he has tried me, I shall come forth
> as gold" (Job 23:10).
>
> Luke Plant || http://lukeplant.me.uk/
>
> --
>
> You received this message because you are subscribed to the Google Groups "Django developers" group.
> To post to this group, send email to django-d...@googlegroups.com.
> To unsubscribe from this group, send email to django-develop...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
>
>
>

Alex

--
"I disapprove of what you say, but I will defend to the death your
right to say it." -- Voltaire
"The people's good is the highest law." -- Cicero
"Code can always be simpler than you think, but never as simple as you
want" -- Me

SmileyChris

unread,
Nov 29, 2009, 4:12:21 AM11/29/09
to Django developers
On Nov 29, 6:40 am, Luke Plant <L.Plant...@cantab.net> wrote:
> Hi all,
>
> I started work replacing Django's if with the "smart-if" template tag
> by Chris Beaven (http://www.djangosnippets.org/snippets/1350/)

Neat! I'm assuming you'll be posting this to your bitbucket soon? :)

> 1) Handling non-existent variables
> 2) TEMPLATE_STRING_IF_INVALID breaks everything

Yep, this is a bit of a problem which I've been thinking about
recently (encountered it for a project).

My (non-implemented) solution to both of these is to use a NotFound
object. I just coded one up: http://gist.github.com/244861
This could actually be a much better return value than a plain
TEMPLATE_STRING_IF_INVALID string in general.

> 3) Behaviour of 'x and b or y'

Like I commented in the snippet, I don't think it's worth obfuscating
code to limit this behaviour.

> 4) Behaviour of 'not'

Bleh, whatever. If we need it for backwards compatibility, it's not
hard to implement.

Russell Keith-Magee

unread,
Nov 30, 2009, 6:18:13 AM11/30/09
to django-d...@googlegroups.com
On Sun, Nov 29, 2009 at 1:40 AM, Luke Plant <L.Pla...@cantab.net> wrote:
> Hi all,
>
> I started work replacing Django's if with the "smart-if" template tag
> by Chris Beaven ( http://www.djangosnippets.org/snippets/1350/ )
>
> Of course, it is not as simple as it looks...
>
> 4 issues:
>
> 1) Handling non-existent variables
> ==================================
>
> The current smart-if does not handle the case of non-existent
> variables like Django does.  (For example, something like
>
>  {% if foo or bar %}
>
> when one of foo or bar is not defined in the context.)
>
> To do this properly, I think the best approach would be something like
> "almost three valued logic", which would work as follows:
>
> 1) variables that don't exist are represented by Null
> 2) all operators return either True or False, never Null
> 3) for 'not', 'and' and 'or', Null effectively acts like False
>   (so "not Null" == True, "Null or True" == True etc)
> 4) for all comparison operators, if either argument is Null then
>   the result is False e.g. "Null == Null" is False
>   "Null > 1" is False, "Null <= 1" is False, except
>   "!=", which should return true if either value is Null.
>
> Any comments on this approach? I've started to implement this, and it
> works OK so far, just needs a bit more work.

I was with you right up until the equality comparisons (Null == Null
-> False etc). As noted by Alex, it conflicts with the answer given by
{% ifequal %}; it also differs with Python's behavior.

> 2) TEMPLATE_STRING_IF_INVALID breaks everything
> ===============================================
>
> The smart 'if' tag allows for filters:
>
> {% if articles|length >= 5 %}
>
> To achieve this, it puts its arguments through FilterExpression.  If
> settings.TEMPLATE_STRING_IS_INVALID is not an empty string, this means
> that:
>
> {% if foo %}blah{% endif %}
>
> will render 'blah' instead of '' if 'foo' is not in the context.  This
> causes test failures at the moment.  We could change the tests, but
> that is a backwards incompatibility, and it highlights a fundamental
> change in the behaviour of {% if %}, which basically makes
> TEMPLATE_STRING_IF_INVALID useless for even its stated purpose of
> debugging, because the logic of a template will now be changed.
>
> Personally, I care nothing for TEMPLATE_STRING_IF_INVALID, as there
> are other things that it breaks, and I never use it. But obviously I
> can't just think of myself here.
>
> If there was an easy solution that didn't change the behaviour of the
> if tag in this case that would be great, but I can't see one.

The resolve call on FilterExpression takes an "ignore_failures"
argument that exists for exactly this purpose - contrib.comments uses
this setting to ensure that generic content type lookups don't end up
with spurious data. I suspect the same argument should be usable here,
too.

> 3) Behaviour of 'x and b or y'
> ==============================
>
> Previously this was a syntax error.  In Chris's code, it is fine and
> works like Python. I would vote for keeping the smart-if as it is, as
> I don't see the value of complicating the implementation to disallow
> something that could be useful sometimes.

I agree. While I still think having excessive logic in the template is
a bad idea, I don't see too much harm in providing the capability on
the off chance it is legitimately useful. This is double so given that
preventing complex logic will complicate the implementation, and
almost inevitably cause grief from those users that don't understand
why the limitation is in place.

The kink in this particular plan is that it will require clearly
documenting the order of evaluation of logical operators - especially
since parentheses aren't available for explicit clarification.

> 4) Behaviour of 'not'
> =====================
> Current Django will interpret 'not' as a variable if there is no
> ambiguity e.g.
>
>   {% if not %}blah{% endif %}
>
> This is not documented, but the behaviour is specified in detail in
> the tests, so changing it would be a backwards incompatibility. The
> smart-if is different (the above is a syntax error), and I suspect
> that making the smart-if behave like current Django could complicate
> things a lot.
>
> IMO, current Django behaviour here is complete misfeature! I don't
> know of any other language where keywords can be treated as variables
> if the keyword doesn't make sense in that position...

I completely agree that this is a nasty wart, but I'm not completely
convinced that it should count as a backwards incompatible change. We
don't document this particular quirk of the if tag, and I don't think
we've ever claimed that the test suite is the ultimate arbiter of
correctness. After all, the test suite can have errors, and the wrong
behavior is still wrong, even if the wrongness is tested and
validated.

Personally, I think this falls into the same category as the
introduction of the 'permanent' argument on the redirect_to generic
view. Strictly, that was backwards incompatible for anyone that was
using the redirect_to view with a format string key of 'permanent'.
Similar logic applies here - it's an edge case of very poor template
variable choice. Obviously, we should document it given that it is a
known edge case that will cause problems, but I don't think that
should stop us from doing the right thing (and, more to the point,
stopping people from continuing to do the wrong thing).

Yours,
Russ Magee %-)

Luke Plant

unread,
Nov 30, 2009, 12:08:50 PM11/30/09
to django-d...@googlegroups.com
On Monday 30 November 2009 11:18:13 Russell Keith-Magee wrote:

> > 1) Handling non-existent variables
> > ==================================
...
> > To do this properly, I think the best approach would be something
> > like "almost three valued logic", which would work as follows:
> >
> > 1) variables that don't exist are represented by Null
> > 2) all operators return either True or False, never Null
> > 3) for 'not', 'and' and 'or', Null effectively acts like False
> > (so "not Null" == True, "Null or True" == True etc)
> > 4) for all comparison operators, if either argument is Null then
> > the result is False e.g. "Null == Null" is False
> > "Null > 1" is False, "Null <= 1" is False, except
> > "!=", which should return true if either value is Null.
> >
> > Any comments on this approach? I've started to implement this,
> > and it works OK so far, just needs a bit more work.
>
> I was with you right up until the equality comparisons (Null ==
> Null -> False etc). As noted by Alex, it conflicts with the answer
> given by {% ifequal %}; it also differs with Python's behavior.

Yeah, I hadn't thought of that. I don't think it's a case of differing
with Python's behaviour, as we are making a deliberate choice to
substitute a missing variable with None (or Null or whatever). You
could argue that Python throws a NameError in this case.

I was trying to think about what is intuitive. e.g. what should happen
here if foo has not been supplied:

{% if foo < 1 %}
yes
{% endif %}

I think it is fairly counter-intuitive for a template author that you
get 'yes' here. However, on further reflection, the "almost 3 valued
logic" can be equally counter-intuitive, say with the following case:

{% if foo < 1 %}
yes
{% else %}
no
{% endif %}

It would render 'no' if foo was missing, but it might be quite
surprising that if you decided to re-arrange the template by inverting
the logical condition:

{% if foo >= 1 %}
no
{% else %}
yes
{% endif %}

then you get different behaviour. (i.e. 'yes' if foo was missing).

So, I'm *now* suggesting that we convert everything missing to None.

In fact, I've found that doing anything apart from this would be hard.

(Alex, you were right about what Malcolm had done with
FilterExpression, which resolves the problem with
TEMPLATE_STRING_IF_INVALID, but this gives me None instead of '', and
not information about whether the variable actually exists, which is a
slightly different question. I didn't find this out earlier because
most of the tests are against the 'IfParser' which is a subclass of
'TemplateIfParser', and works slightly differently, and because I
wasn't thinking properly).

Behaviour of 'not' - revisited
==============================

I think this leaves just the behaviour of 'not'. It is a bit more
complicated than Alex suggested — the tests include "if not" and "if
not not", and with the current behaviour you can do things like "if
foo and not not" etc.

In fact, you can do it (to some extent) with *any* of the keywords
e.g. "if and", "if x and or". But not always e.g. "if not or and x"
- that, illogically, is a syntax error.

I suspect that allowing everything that was possible before will be
extremely difficult, because the current behaviour is very badly
defined for these edge cases — it works just fine if you do sensible
things like not using variables called 'not', 'and' or 'or', but
working out the rules for the exceptional cases will be very hard.

I propose a backwards incompatibility here, it looks like this:

The 'if' tag no longer accepts 'and', 'or' and 'not' as valid
variable names. Previously that was allowed in some cases even
though they were normally treated as keywords. Now, the keyword
status is always enforced, and code like {% if not %} or {% if and %}
will throw a TemplateSyntaxError.

This would now be the only backwards incompatibility (discounting the
possibility that people were relying on TemplateSyntaxError for things
that are now legal).

Patch
=====

It can be tracked in my 'lp-smartif' branch here:
http://bitbucket.org/spookylukey/django-trunk-lukeplant/

Currently various tests about 'not' are failing, and docs are not
written.

Chris: in the course of my attempted 'Null' behaviour changes, I
changed your implementation slightly — I added explicit classes for
Less, LessOrEqual, NotEqual and Not, pulling out the 'negate'
behaviour from the individual classes (it made implementing the Null
logic simpler). Other than that, very little of the parsing has
changed. Given that the 'Null' stuff has now been removed, we could
move back to your way to reduce the code a bit, but I'm not sure it is
worth it.

Review would be welcome, especially as I'm ill at the moment. I'm only
coding because the boredom of doing nothing is killing me...

Luke

--
"Humiliation: The harder you try, the dumber you look."
(despair.com)

Luke Plant || http://lukeplant.me.uk/

SmileyChris

unread,
Nov 30, 2009, 3:26:30 PM11/30/09
to Django developers
On Dec 1, 6:08 am, Luke Plant <L.Plant...@cantab.net> wrote:
> > I was with you right up until the equality comparisons (Null ==
> >  Null -> False etc). As noted by Alex, it conflicts with the answer
> >  given by {% ifequal %}; it also differs with Python's behavior.
>
> Yeah, I hadn't thought of that. I don't think it's a case of differing
> with Python's behaviour, as we are making a deliberate choice to
> substitute a missing variable with None (or Null or whatever).  You
> could argue that Python throws a NameError in this case.

Personally, I think that "(not found)" should *not* equal "None", even
if that differs from ifequal.

But whatever, I'm not too worried either way.

SmileyChris

unread,
Nov 30, 2009, 3:27:32 PM11/30/09
to Django developers
On Dec 1, 6:08 am, Luke Plant <L.Plant...@cantab.net> wrote:

> Given that the 'Null' stuff has now been removed, we could
> move back to your way to reduce the code a bit, but I'm not sure it is
> worth it.

I'd say reduce the code again.

And I think you missed adding some files in your repo?

Luke Plant

unread,
Nov 30, 2009, 6:17:48 PM11/30/09
to django-d...@googlegroups.com
On Monday 30 November 2009 20:27:32 SmileyChris wrote:
> On Dec 1, 6:08 am, Luke Plant <L.Plant...@cantab.net> wrote:
> > Given that the 'Null' stuff has now been removed, we could
> > move back to your way to reduce the code a bit, but I'm not sure
> > it is worth it.
>
> I'd say reduce the code again.

I've removed Less, LessOrEqual and NotEqual. I kept 'Not' as I think
it's slightly cleaner than putting 'negate' into BaseCalc. It also
removes the asymmetry of using 'Or' at one point in the code, which I
found slightly puzzling.

> And I think you missed adding some files in your repo?

Doh! Added now, along with some docs.

Regards,

Russell Keith-Magee

unread,
Nov 30, 2009, 8:55:48 PM11/30/09
to django-d...@googlegroups.com
On Tue, Dec 1, 2009 at 1:08 AM, Luke Plant <L.Pla...@cantab.net> wrote:
> On Monday 30 November 2009 11:18:13 Russell Keith-Magee wrote:
>
>> > 1) Handling non-existent variables
>> > ==================================
> So, I'm *now* suggesting that we convert everything missing to None.

Sounds good to me.

> Behaviour of 'not' - revisited
> ==============================
>
> I propose a backwards incompatibility here, it looks like this:
>
>  The 'if' tag no longer accepts 'and', 'or' and 'not' as valid
>  variable names.  Previously that was allowed in some cases even
>  though they were normally treated as keywords.  Now, the keyword
>  status is always enforced, and code like {% if not %} or {% if and %}
>  will throw a TemplateSyntaxError.
>
> This would now be the only backwards incompatibility (discounting the
> possibility that people were relying on TemplateSyntaxError for things
> that are now legal).

Also sounds good.

> Patch
> =====
>
> Review would be welcome, especially as I'm ill at the moment. I'm only
> coding because the boredom of doing nothing is killing me...

I'll try and take a look over the next couple of days.

Russ %-)

Russell Keith-Magee

unread,
Dec 5, 2009, 10:39:03 AM12/5/09
to django-d...@googlegroups.com
Here's the review I promised. First the minor points:

* Line 814 of templates/defaulttags.py has a wierd UTF-8 character -
an emdash, rather than an ASCII hyphen. I don't know what's happening
on your system, but this causes crashes for me complaining about
PEP-0263 problems.

* The if-tag-not-02/05 tests that have been removed should be
retained, but be testing for the TemplateSyntaxError (just like the
if-tag-error tests do). If we're going to implement a new language
contract and introduce some reserved words, we should test that that
those words are actually reserved.

* I'm not wild about the docs for the if tag referring how
comparisons work "just like Python operators". We have historically
made a big deal of templates not being Python, and while we are
blurring the lines a little with this patch, I'd still like to
maintain the pretense. The preceding notes about logical operators is
a good example of how it should be done IMHO - it gives examples of
how and/or work without needing to resort to saying "just like
Python".

* The explanation of why {% if a > b > c %} doesn't work is a bit
vague. At the very least, it should have an example of the correct
implmentation - {% if a > b and b > c %}.

* The interaction between this patch and #6262 will be fun. (This is
more a mental note to myself that I should commit #6262)

Now the major issues: There's only one that I found - the parsing
strategy for complex logical expressions. "A or B and C" is parsed as
"(A or B) and C", not the pythonic way "A or (B and C)" (giving
operator precedence to AND over OR).

Personally, I found this very surprising. When I said in a previous
email that I didn't think it was worth hobbling the if statement to
prevent complex logical operations, I presumed it was going to be
replaced with a full parser. Historically, it hasn't been an issue
because we've only allowed one type of logical operator in an {% if
%}. I think I'd rather see this tradition continued rather than allow
mixing in a way that will be confusing to anyone with prior
programming experience.

Lastly, a controversial topic, but I think we need an answer on this -
whither {% ifequal %}? Should we be deprecating it? Given the current
level of usage of the ifequal/ifnotequal tags, this seems excessive.
Perhaps we should consider this for long-term deprecation (i.e., flag
it as a 2.0 deprecation)?

Russ %-)

Luke Plant

unread,
Dec 5, 2009, 3:09:21 PM12/5/09
to django-d...@googlegroups.com
First, thanks for your review Russell, I appreciate that you've been
doing tons of work on lots of things!

On Saturday 05 December 2009 15:39:03 Russell Keith-Magee wrote:

> Here's the review I promised. First the minor points:
>
> * Line 814 of templates/defaulttags.py has a wierd UTF-8 character
> - an emdash, rather than an ASCII hyphen. I don't know what's
> happening on your system, but this causes crashes for me
> complaining about PEP-0263 problems.

I did add a 'coding' line to fix that, but perhaps after you pulled my
latest code. Anyway, it's not worth it so I removed it.

> * The if-tag-not-02/05 tests that have been removed should be
> retained, but be testing for the TemplateSyntaxError (just like the
> if-tag-error tests do). If we're going to implement a new language
> contract and introduce some reserved words, we should test that
> that those words are actually reserved.

There are actually other tests that check this, in the IfParser tests.
I didn't see the usefulness of 5 different tests all checking that
'not' is a keyword, but renumbering the following tests would be
worse, hence the note about why they were missing. Also, it makes more
sense to put the syntax error tests together.

> * I'm not wild about the docs for the if tag referring how
> comparisons work "just like Python operators". We have historically
> made a big deal of templates not being Python, and while we are
> blurring the lines a little with this patch, I'd still like to
> maintain the pretense. The preceding notes about logical operators
> is a good example of how it should be done IMHO - it gives
> examples of how and/or work without needing to resort to saying
> "just like Python".

Fixed now, hopefully, I've given examples for all the operators.

> * The explanation of why {% if a > b > c %} doesn't work is a bit
> vague. At the very least, it should have an example of the correct
> implmentation - {% if a > b and b > c %}.

Fixed now.

> Now the major issues: There's only one that I found - the parsing
> strategy for complex logical expressions. "A or B and C" is parsed
> as "(A or B) and C", not the pythonic way "A or (B and C)" (giving
> operator precedence to AND over OR).
>
> Personally, I found this very surprising. When I said in a previous
> email that I didn't think it was worth hobbling the if statement to
> prevent complex logical operations, I presumed it was going to be
> replaced with a full parser. Historically, it hasn't been an issue
> because we've only allowed one type of logical operator in an {% if
> %}. I think I'd rather see this tradition continued rather than
> allow mixing in a way that will be confusing to anyone with prior
> programming experience.

Good catch. I think it would be nicer to fix than just disallow,
although it could be a substantial change to the way the IfParser
class works. I don't think it is too much work, however, because the
hard bit is tokenizing, which has already been done.

I'm not likely to able to look at this before Tuesday. If anyone
wants to look at it, I think the right approach is something like the
following:
http://effbot.org/zone/simple-top-down-parsing.htm
(without the globals, obviously, they can be converted to instance
variables/methods).

I don't think trying to hack this on to the current IfParser would be
a good idea. IfParser already has a slightly hacky way of handling
the precedence of 'and' and 'or' relative to the other operators, and
this is a good opportunity to replace that with something nicer.

> Lastly, a controversial topic, but I think we need an answer on
> this - whither {% ifequal %}? Should we be deprecating it? Given
> the current level of usage of the ifequal/ifnotequal tags, this
> seems excessive. Perhaps we should consider this for long-term
> deprecation (i.e., flag it as a 2.0 deprecation)?

Yes, I see no value in deprecating this until 2.0, given how much it
is used.

However, while I remember, there is this bug which we may need to
revisit:

http://code.djangoproject.com/ticket/10975

Essentially, you can't use {% block %} inside an {% if %} tag, but you
can in an {% ifequal %} tag. Irrespective of which one is right, it
seems unreasonable that {% if x == y %} behaves so differently from
{% ifequal x y %} in this regard. (Actually, I didn't test that.
Perhaps the behaviour of 'if' has changed with the smart if code).

Regards,

Luke


--
"Idiocy: Never underestimate the power of stupid people in large
groups." (despair.com)

Luke Plant || http://lukeplant.me.uk/

Luke Plant

unread,
Dec 5, 2009, 8:07:16 PM12/5/09
to django-d...@googlegroups.com
On Saturday 05 December 2009 20:09:21 Luke Plant wrote:

> I'm not likely to able to look at this before Tuesday. If anyone
> wants to look at it, I think the right approach is something like
> the following:
> http://effbot.org/zone/simple-top-down-parsing.htm
> (without the globals, obviously, they can be converted to instance
> variables/methods).

Cancel that - I unexpectedly had free time this evening, and I
implemented this. It's a nice replacement I think (credit to Vaughan
Pratt and Fredrik Lundh for the basic approach and Python
implementation respectively). The new implementation is pretty much
the same length as the old one, and hopefully easier to read, with a
much smaller core parser. Precedence is specified directly, rather
than implicitly, so it's much easier to check that it's the same as
Python's.

Latest patch attached to this e-mail.
smartif_8.diff

SmileyChris

unread,
Dec 6, 2009, 3:32:50 AM12/6/09
to Django developers
Because that link intrigued me, I challenged myself to write my own
generic lexer & parser based on what I had read:
http://gist.github.com/250128
>  smartif_8.diff
> 27KViewDownload

Alex Gaynor

unread,
Dec 6, 2009, 3:35:52 AM12/6/09
to django-d...@googlegroups.com
The undcoumented scanner class in the regex module may interest you:
http://code.activestate.com/recipes/457664/

Alex
> --
>
> You received this message because you are subscribed to the Google Groups "Django developers" group.
> To post to this group, send email to django-d...@googlegroups.com.
> To unsubscribe from this group, send email to django-develop...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
>
>
>



Luke Plant

unread,
Dec 8, 2009, 8:17:53 PM12/8/09
to django-d...@googlegroups.com
Hi all,

I've now addressed everything in Russell's last e-mail, I think, so I
think I'm pretty much good to go, apart from:

1) my last change rewrote a lot of IfParser, which was the heart of
the patch. That means it probably needs looking at again! On the other
hand, the new implementation is based on a much more general and more
tested parser methodology, so it should hopefully be pretty solid.

2) what should be done about errors with operators? For the most
part, it isn't much of an issue, as 'object' defines many of the magic
methods involved. For 'in', however, you are likely to get TypeErrors
if the object doesn't support it. Currently missing variables are
handled gracefully, but {% if 'x' in 1 %} will get you a TypeError
that is not caught. And there is still the possibility that other
operators will cause errors.

Our current policies don't quite handle this situation, AFAICS. For
the case of 'in', catching any Exception and returning False could be
defended - to use the above example, 1 is not an object that contains
'x', therefore it should return False. This would protect template
authors against various exceptions.

Or we could go with the policy for method calls, which is to check the
exception for 'silent_variable_failure' attribute. This has the
advantage that inappropriate use of 'in' by template authors, which is
almost always going to be a mistake, will throw a loud error. The
error message in this case isn't particularly obvious though:
"TypeError: argument of type 'int' is not iterable"

I'm leaning towards the latter. If a view changes so that, in the
variables passed to the template, a container with a __contains__()
method is replaced by one without, or by one with a buggy
__contains__() method that throws exceptions, it's more useful and
arguably more correct to get an exception than for the 'if' tag
expression to return False.

Regards,

Luke

--
"If something is hard, it's not worth doing." (Homer Simpson)

Luke Plant || http://lukeplant.me.uk/

Russell Keith-Magee

unread,
Dec 9, 2009, 7:43:04 AM12/9/09
to django-d...@googlegroups.com
On Wed, Dec 9, 2009 at 9:17 AM, Luke Plant <L.Pla...@cantab.net> wrote:
> Hi all,
>
> I've now addressed everything in Russell's last e-mail, I think, so I
> think I'm pretty much good to go, apart from:
>
> 1) my last change rewrote a lot of IfParser, which was the heart of
> the patch. That means it probably needs looking at again! On the other
> hand, the new implementation is based on a much more general and more
> tested parser methodology, so it should hopefully be pretty solid.

I've done another review, and it's looking pretty good. I have three
relatively minor comments:

* There is what appears to be vestigial documentation around line 346
of ref/templates/builtins.txt; it says evaluation order is
left-to-right, rather than operator precedence as described later on.

* Most of the lambdas you have defined in smartif.py are already
available in the builtin operator module - e.g., operator.or_,
operator.and_, operator.gt, etc.

* The handling of nodelist_false in IfNode isn't wrong (as in, it
doesn't generate wrong results), but it was contrary to my
expectations. I would have expected a simple "else:
self.nodelist_false.render()". Is there a particular reason for this?
I made the change in source and it didn't break any tests.

The only two reasons I could come up with to explain this approach
were that it would be marginally faster to return '' rather than
render an empty nodelist, and/or an old-school mandate that every
function should have a return statement at the end. I'm not
necessarily saying this should be changed; but at the very least, it
warrants an explanatory comment as to why the obvious approach isn't
being used.

Other than the issue you raise below, I'm happy. In particular, I'm
much happier with the parsing code now - I found it much less
confusing that the previous iteration, and it has the added benefit of
being correct :-)

> 2) what should be done about errors with operators?  For the most
> part, it isn't much of an issue, as 'object' defines many of the magic
> methods involved.  For 'in', however, you are likely to get TypeErrors
> if the object doesn't support it. Currently missing variables are
> handled gracefully, but {% if 'x' in 1 %} will get you a TypeError
> that is not caught.  And there is still the possibility that other
> operators will cause errors.
>
> Our current policies don't quite handle this situation, AFAICS.  For
> the case of 'in', catching any Exception and returning False could be
> defended - to use the above example, 1 is not an object that contains
> 'x', therefore it should return False.  This would protect template
> authors against various exceptions.
>
> Or we could go with the policy for method calls, which is to check the
> exception for 'silent_variable_failure' attribute. This has the
> advantage that inappropriate use of 'in' by template authors, which is
> almost always going to be a mistake, will throw a loud error. The
> error message in this case isn't particularly obvious though:
> "TypeError: argument of type 'int' is not iterable"
>
> I'm leaning towards the latter. If a view changes so that, in the
> variables passed to the template, a container with a __contains__()
> method is replaced by one without, or by one with a buggy
> __contains__() method that throws exceptions, it's more useful and
> arguably more correct to get an exception than for the 'if' tag
> expression to return False.

This is a hard one. Keeping with our tradition of disagreeing with
each other, I'm actually inclined to go the other way :-)

I have two reasons. Firstly, I'm of the opinion that templates
shouldn't ever throw exceptions during rendering - they should fail
silently. Differentiating between False and Exception may be difficult
or inconvenient for the developer, but it's even more inconvenient for
the end user to see a 500 IMHO.

Secondly, while I can see the analog with silent_variable_failure, it
isn't quite the same circumstances. Most of the interesting functions
that will be invoked in templates will be internal to Django or
user-code, so we have almost full control over the
silent_variable_failure attribute. We can set silent_variable_failure
on DoesNotExist because it is internal to Django, and it will behave
nicely when errors occur in templates.

However, to my reckoning, this isn't true for operators. Many, if not
most of the interesting use cases for operators will be comparisons
with literals, sets, lists, or other Python builtins - which throw
exceptions which we can't control. We may well honor a
silent_variable_failure attribute, but if it's near impossible to
actually set that attribute in practice, there isn't much point in
honoring it in the first place.

Yours,
Russ Magee %-)

Luke Plant

unread,
Dec 9, 2009, 8:27:24 AM12/9/09
to django-d...@googlegroups.com
On Wednesday 09 December 2009 12:43:04 Russell Keith-Magee wrote:

> I've done another review, and it's looking pretty good. I have
> three relatively minor comments:
>
> * There is what appears to be vestigial documentation around line
> 346 of ref/templates/builtins.txt; it says evaluation order is
> left-to-right, rather than operator precedence as described later
> on.

Thanks, changed.

> * Most of the lambdas you have defined in smartif.py are already
> available in the builtin operator module - e.g., operator.or_,
> operator.and_, operator.gt, etc.

Oh yeah, forgot about those!

> * The handling of nodelist_false in IfNode isn't wrong (as in, it
> doesn't generate wrong results), but it was contrary to my
> expectations. I would have expected a simple "else:
> self.nodelist_false.render()". Is there a particular reason for
> this? I made the change in source and it didn't break any tests.
>
> The only two reasons I could come up with to explain this approach
> were that it would be marginally faster to return '' rather than
> render an empty nodelist, and/or an old-school mandate that every
> function should have a return statement at the end. I'm not
> necessarily saying this should be changed; but at the very least,
> it warrants an explanatory comment as to why the obvious approach
> isn't being used.

I don't know the reason either, it's part of Chris' original patch,
and I've simplified it now.

> This is a hard one. Keeping with our tradition of disagreeing with
> each other, I'm actually inclined to go the other way :-)
>
> I have two reasons. Firstly, I'm of the opinion that templates
> shouldn't ever throw exceptions during rendering - they should fail
> silently. Differentiating between False and Exception may be
> difficult or inconvenient for the developer, but it's even more
> inconvenient for the end user to see a 500 IMHO.

You're right, and I was misreading the current code in thinking that
it currently raises exceptions during rendering. While Variable and
others raise exceptions, they are caught higher up. (There are just
occasional places where builtins propagate exceptions, like {% url
%}).

> Secondly, while I can see the analog with silent_variable_failure,
> it isn't quite the same circumstances. Most of the interesting
> functions that will be invoked in templates will be internal to
> Django or user-code, so we have almost full control over the
> silent_variable_failure attribute. We can set
> silent_variable_failure on DoesNotExist because it is internal to
> Django, and it will behave nicely when errors occur in templates.
>
> However, to my reckoning, this isn't true for operators. Many, if
> not most of the interesting use cases for operators will be
> comparisons with literals, sets, lists, or other Python builtins -
> which throw exceptions which we can't control. We may well honor a
> silent_variable_failure attribute, but if it's near impossible to
> actually set that attribute in practice, there isn't much point in
> honoring it in the first place.

Very good point. You convinced me easily, contrary to tradition this
time :-)

Thanks for the review again.

Luke

--
If you can't answer a man's arguments, all is not lost; you can
still call him vile names. (Elbert Hubbard)

Luke Plant || http://lukeplant.me.uk/
Reply all
Reply to author
Forward
0 new messages