Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#17229 closed New feature (fixed)

Allow 'True', 'False' and 'None' to resolve to corresponding Python objects

Reported by: anatoly techtonik <techtonik@…> Owned by: Aymeric Augustin
Component: Template system Version: 1.2
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description (last modified by Luke Plant)

[Original bug title: Invalid '==' expression silently ignored leading to invalid result]

Let me clarify the title - I am not sure the expression below is invalid. It is the expression I intuitively expect to work. But perhaps Django it is a specifics of template language. In any case it should either complain or produce expected output, which it doesn't.

_{{ closed }}_
{% if closed == True %} ***** {% endif %}

If closed variable is equal to None, the example above will output the string:

_None_ *****

Expected is to output nothing or to give a warning about invalid right-side argument.

If closed is boolean (not string) and is equal to True, it will output

_True_

Expected is to output

_True_ *****

or to give a warning about invalid right-side argument.

Attachments (2)

17229.patch (489 bytes) - added by Aymeric Augustin 5 years ago.
17229.2.patch (4.3 KB) - added by Aymeric Augustin 5 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 5 years ago by Luke Plant

Resolution: invalid
Status: newclosed

Your example is invalid, because 'True' is not a literal that is understood by the template language. Rather, the string 'True' in your template is interpreted as a variable name. Since a variable called "True" does not exist in the context, the behaviour you see above follows (though the full sequence of steps is a little involved).

The expression you want is simply:

{% if closed %}

If you want the negation:

{% if not closed %}

(This, by the way, is normally considered good style in Python as well. Adding the no-op comparison == True is pointless - and so neither the majority of Python developers nor template writers would intuitively write this).

comment:2 Changed 5 years ago by Luke Plant

By the way, the template language does not give warnings for missing variables. By design it succeeds silently, and on rare occasions throws exceptions.

comment:3 in reply to:  1 Changed 5 years ago by anatoly techtonik <techtonik@…>

Needs documentation: set
Resolution: invalid
Status: closedreopened

Replying to lukeplant:

Your example is invalid, because 'True' is not a literal that is understood by the template language.

This explains the behavior, but doesn't solve the issue.

Rather, the string 'True' in your template is interpreted as a variable name. Since a variable called "True" does not exist in the context, the behaviour you see above follows (though the full sequence of steps is a little involved).

Don't you think that Pythonic template engine should allow standard Python True/False keywords in comparison expressions? There is a clear need to use them for NullBooleanField comparisons, which is impossible otherwise (which is my case, btw, that your example below doesn't solve). The only reason not to teach Django understand True/False literals is to allow people set these variables in templates themselves, but I really don't think you'd like to encourage such practice.

The expression you want is simply:

{% if closed %}

If you want the negation:

{% if not closed %}

(This, by the way, is normally considered good style in Python as well. Adding the no-op comparison == True is pointless - and so neither the majority of Python developers nor template writers would intuitively write this).

That's not what I want. In my case closed is NullBooleanField used as a filter for issue list. None means that both open and closed issues should be returned. I need to distinguish between None and False.

comment:4 Changed 5 years ago by Simon Charette

I know it might feel a bit hackish but I think you get work around this limitation by using the default_if_none filter.

{% if closed|default_if_none:'None' == 'None' %}
  None
{% else %}{% if closed %}
  True
{% else %}
  False
{% endif %}{% endif %}

comment:5 Changed 5 years ago by Luke Plant

Description: modified (diff)
Needs documentation: unset
Summary: Invalid '==' expression silently ignored leading to invalid resultAllow 'True', 'False' and 'None' to resolve to corresponding Python objects
Triage Stage: UnreviewedDesign decision needed
Type: BugNew feature
UI/UX: unset

Your initial example only did a comparison to True, and didn't mention any comparison to None. I am not a mind reader!

The ticket has now changed in scope to the feature addition "Allow 'True', 'False' and 'None' to resolve to corresponding Python objects".

I can see why this would be useful, but I'm not entirely convinced, given that we've survived without these for a long time. Therefore marking DDN.

comment:6 Changed 5 years ago by Luke Plant

The other thing to add is that comparison to 'None' does work (by accident, really). If you write:

{% if closed == None %}
  None
{% else %}{% if closed %}
  True
{% else %}
  False
{% endif %}{% endif %}

then you get the behaviour you want.

comment:7 in reply to:  5 Changed 5 years ago by anatoly techtonik <techtonik@…>

Replying to lukeplant:

Your initial example only did a comparison to True, and didn't mention any comparison to None. I am not a mind reader!

I couldn't understand the magic behavior and where the bug is, because documentation doesn't warn that you can't use Python True/False literals in expressions that look like usual Python expressions (I also could not understand why would anyone limit people from using them). So, as a result of this report I expected the way to compare booleans to be at least documented if not fixed, because after spending several hours trying to debug this stuff, you start to understand people sending rays of hate to Django templating system. :)

My frustration with the fact that I don't understand how to compare booleans (when you can compare integers) is the reason I selected UI/UX checkbox several times. When I understood that it is not the boolean story alone, I split the user story into two. This one about True/False alone, and #17230 for None bug.

I can see why this would be useful, but I'm not entirely convinced, given that we've survived without these for a long time. Therefore marking DDN.

I anticipated such reaction when it became clear in #17230 that the bug is actually there and was there for a long time, so thanks for not closing it immediately as 'works for me'.

My workaround (which I feel is still hackish and not really intuitive) is:

  {%if closed|lower == 'false' %} (opened only) {%endif%}
  {%if closed|lower == 'true' %} (closed only) {%endif%}

comment:8 Changed 5 years ago by Aymeric Augustin

Triage Stage: Design decision neededAccepted

I classified Python builtins with the script below and I can confirm that True, False and None are the only ones that would make sense in templates. There's no risk of feature creep here.

The Django template language is primarily designed for designers, but it's also widely used by Python programmers. The principle of least surprise tells that we should make these three values builtins of the template language too. It just feels wrong that True evaluates silently to None.


def qualify(thing):
    if isinstance(thing, BaseException):
        return 'exc'
    elif type(thing) == type:
        return 'cls'
    elif type(thing) == type(map):
        return 'fun'
    else:
        return 'other: ' + type(thing).__name__

for b in dir(__builtins__):
    print '%30s  %s' % (b, qualify(getattr(__builtins__, b)))

comment:9 Changed 5 years ago by Aymeric Augustin

Easy pickings: set
Has patch: set
Needs documentation: set
Needs tests: set

I'm attaching a patch showing one way to implement this.

Changed 5 years ago by Aymeric Augustin

Attachment: 17229.patch added

comment:10 Changed 5 years ago by Aymeric Augustin

comment:11 Changed 5 years ago by Aymeric Augustin

carljm> you have my agreement on #17229, I think it has no downsides and fixes something that is likely to surprise (from #django-dev)

comment:12 Changed 5 years ago by Aymeric Augustin

Owner: changed from nobody to Aymeric Augustin
Status: reopenednew

comment:13 Changed 5 years ago by Aymeric Augustin

This is arguably a new feature of the template engine, so it'll have to wait until 1.4 is out.

comment:14 Changed 5 years ago by anatoly techtonik <techtonik@…>

Does that mean it won't be included in 1.4? Oh my.

comment:15 Changed 5 years ago by Aymeric Augustin

Well -- it could have been if you had provided a suitable patch (with tests and docs).

comment:16 Changed 5 years ago by anatoly techtonik <techtonik@…>

Well, if it wasn't a bug in the first place - I probably felt too lazy even to submit it. =)

Changed 5 years ago by Aymeric Augustin

Attachment: 17229.2.patch added

comment:17 Changed 5 years ago by Aymeric Augustin

Needs documentation: unset
Needs tests: unset

comment:18 Changed 5 years ago by Aymeric Augustin

Resolution: fixed
Status: newclosed

In [17894]:

Fixed #17229 -- Allow 'True', 'False' and 'None' to resolve to the corresponding Python objects in templates.

comment:19 Changed 5 years ago by anatoly techtonik <techtonik@…>

Awesome. Nice to see it is going to be in Django 1.5

comment:20 Changed 5 years ago by Claude Paroz

Changeset r17894 produced several errors in the Django test suite. In fact, it reveals issues where Context instances are initialized with a Context argument instead of a simple dict.
Examples:

  • In browser:django/trunk/django/contrib/auth/tests/urls.py, several render_to_response calls have the Context instance as second argument, which seems to be plain bugs (uncaught until now)
  • In browser:django/trunk/django/contrib/admin/templatetags/admin_modify.py, the prepopulated_fields_js inclusion tag returns a Context instance instead of a dict.

The question is now whether Context initialization should accept other Context instance as the __init__ dict_ parameter (and then this changeset should be fixed) or if we should enforce dict_ as a dict and fix places in Django code when it is not the case. I'd be in favour of the latter, but this might need some more investigation.

comment:21 Changed 5 years ago by Aymeric Augustin

Resolution: fixed
Status: closedreopened

comment:22 Changed 5 years ago by Anssi Kääriäinen

It could make sense to just revert r17894 to make trunk work again.

Otherwise, the question is how likely user code is to pass in Context instances. I guess it happens, as it wasn't exactly uncommon in our code either. So, if Context is going to be disallowed, then at least provide a clear error message of what happened.

comment:23 Changed 5 years ago by Aymeric Augustin

Resolution: fixed
Status: reopenedclosed

The regression is tracked in #18103.

comment:24 in reply to:  20 Changed 5 years ago by Aymeric Augustin

Replying to claudep:

Changeset r17894 produced several errors in the Django test suite. In fact, it reveals issues where Context instances are initialized with a Context argument instead of a simple dict.

Claude, I've moved this to its own ticket (#18105) because I'd like to fix the regression ASAP, but keep track of your suggestion.

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