Code

Opened 2 years ago

Closed 2 years ago

Last modified 2 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: aaugustin
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 lukeplant)

[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 aaugustin 2 years ago.
17229.2.patch (4.3 KB) - added by aaugustin 2 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 follow-up: Changed 2 years ago by lukeplant

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

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 2 years ago by lukeplant

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 2 years ago by anatoly techtonik <techtonik@…>

  • Needs documentation set
  • Resolution invalid deleted
  • Status changed from closed to reopened

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 2 years ago by charettes

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 follow-up: Changed 2 years ago by lukeplant

  • Description modified (diff)
  • Needs documentation unset
  • Summary changed from Invalid '==' expression silently ignored leading to invalid result to Allow 'True', 'False' and 'None' to resolve to corresponding Python objects
  • Triage Stage changed from Unreviewed to Design decision needed
  • Type changed from Bug to New 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 2 years ago by lukeplant

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 2 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 2 years ago by aaugustin

  • Triage Stage changed from Design decision needed to Accepted

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 2 years ago by aaugustin

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

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

Changed 2 years ago by aaugustin

comment:11 Changed 2 years ago by aaugustin

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 2 years ago by aaugustin

  • Owner changed from nobody to aaugustin
  • Status changed from reopened to new

comment:13 Changed 2 years ago by aaugustin

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

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

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

comment:15 Changed 2 years ago by aaugustin

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

comment:16 Changed 2 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 2 years ago by aaugustin

comment:17 Changed 2 years ago by aaugustin

  • Needs documentation unset
  • Needs tests unset

comment:18 Changed 2 years ago by aaugustin

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

In [17894]:

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

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

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

comment:20 follow-up: Changed 2 years ago by 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.
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 2 years ago by aaugustin

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:22 Changed 2 years ago by akaariai

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 2 years ago by aaugustin

  • Resolution set to fixed
  • Status changed from reopened to closed

The regression is tracked in #18103.

comment:24 in reply to: ↑ 20 Changed 2 years ago by aaugustin

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.

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.