Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#6398 closed (fixed)

Add optional {% default %} clause to {% for %} template tag

Reported by: Jannis Leidel Owned by: Jannis Leidel
Component: Template system Version: master
Severity: Keywords: easy-pickings
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

As requested by Simon I propose an {% else %} tag for the {% for %} template tag which could be used like this:

from django.template import *
>>> t1 = Template("""
    {% load mytags %}
    {% for athlete in athlete_list %}
        {{ athlete }} 
    {% else %}
        No athlete in list!
    {% endfor %}
    """)
>>> c1 = Context(
    {'athlete_list':
        ['me', 'myself', 'I']
    })
>>> t1.render(c1)
u'me myself I '
>>> c2 = Context({})
>>> t1.render(c2)
u'No athlete in list!'

For a standalone version look at http://www.djangosnippets.org/snippets/546/

Attachments (6)

add_else_to_for.diff (3.8 KB) - added by Jannis Leidel 9 years ago.
First implementation, including documentation
add_default_to_for.diff (3.8 KB) - added by Jannis Leidel 9 years ago.
Second implementation, including documentation
add_default_to_for.2.diff (4.9 KB) - added by Jannis Leidel 9 years ago.
Third implementation, including documentation and tests, fixing some issues, thanks SmileyChris!
add_default_to_for.3.diff (5.6 KB) - added by Jannis Leidel 9 years ago.
Fourth implementation, including documentation and tests. Do you think this is sufficient? Maby I still don't see your point :/
add_default_to_for.4.diff (5.6 KB) - added by Jannis Leidel 9 years ago.
fifth implementation with optional nodelist_default, including documentation and tests.
ticket6398-r9032.diff (5.4 KB) - added by Jannis Leidel 8 years ago.
Updated patch to Django r9032 (http://github.com/jezdez/django/tree/ticket6398)

Download all attachments as: .zip

Change History (33)

Changed 9 years ago by Jannis Leidel

Attachment: add_else_to_for.diff added

First implementation, including documentation

comment:1 Changed 9 years ago by Simon Greenhill <dev@…>

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedReady for checkin

comment:2 Changed 9 years ago by Rob Hudson <treborhudson@…>

The same be accomplished by this:

{% if athlete_list %}
  {% for athlete in athlete_list %}
    ...
  {% endfor %}
{% else %}
  Nothing in list
{% endif %}

comment:3 Changed 9 years ago by anonymous

Triage Stage: Ready for checkinDesign decision needed

comment:4 Changed 9 years ago by anonymous

Such behaviour can be confusing as it is not similar to python

for object in objects:
    if object:
        print 'Found'
        break
else:
    print 'Not found'

pattern.

comment:5 Changed 9 years ago by Adrian Holovaty

Triage Stage: Design decision neededAccepted

The idea gets a +1 from me.

comment:6 Changed 9 years ago by Matthias Pronk <django@…>

I wonder how useful this new syntax is, as you can't use it for the following (pretty common) pattern:

{% if athlete_list %}
  <ul>
  {% for athlete in athlete_list %}
  <li>{{ athlete.name }}</li>
  {% endfor %}
  </ul>
{% else %}
  <p>Nothing in list</p>
{% endif %}

(Because there is no place where you can put the <ul> tags.)

Plus it doesn't comply with the Python syntax (as Anonymous@20:51 says).

comment:7 Changed 9 years ago by Jannis Leidel

Thanks Adrian! Matthias, I think it's still worthwhile and useful if you use:

<ul>
  {% for athlete in athlete_list %}
    <li>{{ athlete.name }}</li>
  {% else %}
    <li class="emptylist">Nothing in list</li>
  {% endfor %}
</ul>

I don't see a reason why compliance with the Python syntax is required for a template tag.

comment:8 Changed 9 years ago by Simon Willison

There's a good discussion about this on my blog, where Fredrik points out that this would be semantically different from how Python handles else: blocks attached to for statements. As such, the extra block would need a different name - {% default %} was suggested.

Changed 9 years ago by Jannis Leidel

Attachment: add_default_to_for.diff added

Second implementation, including documentation

comment:10 Changed 9 years ago by Jannis Leidel

Summary: Add optional {% else %} clause to {% for %} template tagAdd optional {% default %} clause to {% for %} template tag

Ok, this sounds like a reasonable compromise. I changed it to having an optional {% default %} clause.

comment:11 Changed 9 years ago by Jannis Leidel

The working example is:

>>> from django.template import *
>>> t = Template("""
    {% for athlete in athlete_list %}
        {{ athlete }} 
    {% default %}
        No athlete in list!
    {% endfor %}
    """)
>>> c1 = Context(
    {'athlete_list':
        ['me', 'myself', 'I']
    })
>>> t.render(c1)
u'me myself I '
>>> c2 = Context({})
>>> t.render(c2)
u'No athlete in list!'

comment:12 Changed 9 years ago by Chris Beaven

Needs tests: set
Patch needs improvement: set

Still a reference to else in the patch (in a docstring), and needs tests.

I'd suggest dropping the "As you can see, " in the docs and also to mention that the default node is displayed if the given array is empty or not found (emphasis for the bit that I'm suggesting adding).

Fix those and it's good to go.

Changed 9 years ago by Jannis Leidel

Attachment: add_default_to_for.2.diff added

Third implementation, including documentation and tests, fixing some issues, thanks SmileyChris!

comment:13 Changed 9 years ago by Jannis Leidel

Needs tests: unset

comment:14 Changed 9 years ago by Jannis Leidel

Ouh, am I actually allowed to change the ticket properties?

comment:15 Changed 9 years ago by Chris Beaven

If you're confident that you've fixed the issue, sure. It doesn't change it's status so that is alright.

Final thoughts:

  • Perhaps make nodelist_default optional in case someone is using ForNode for thier own devices? def __init__(self, loopvars, sequence, is_reversed, nodelist_loop, nodelist_default=None): (and then move the empty NodeList generation into their too)
  • You should have a test for the actual working case (i.e. non-empty list)
  • You need an empty line after "New in Django development version"

comment:16 Changed 9 years ago by Jannis Leidel

Ok, sounds all reasonable, except one thing: Do you mean with "the empty NodeList generation" the following?

class ForNode(Node):
    ...
    def render(self, context):
        nodelist = NodeList() # <-- this line?
    ...

comment:17 Changed 9 years ago by Chris Beaven

Yea, that logic obviously needs to move if nodelist_default could be None in __init__

Changed 9 years ago by Jannis Leidel

Attachment: add_default_to_for.3.diff added

Fourth implementation, including documentation and tests. Do you think this is sufficient? Maby I still don't see your point :/

comment:18 Changed 9 years ago by Jannis Leidel

Is the last patch what you meant?

comment:19 Changed 9 years ago by Chris Beaven

Close but not quite, you need something like this:

lass ForNode(Node):
    def __init__(self, loopvars, sequence, is_reversed, nodelist_loop, nodelist_default=None):
        self.loopvars, self.sequence = loopvars, sequence
        self.is_reversed = is_reversed
        self.nodelist_loop = nodelist_loop
        if nodelist_default is None:
            self.nodelist_default = NodeList()
        else:
            self.nodelist_default = nodelist_default

comment:20 Changed 9 years ago by Jannis Leidel

Owner: changed from nobody to Jannis Leidel

comment:21 Changed 9 years ago by Chris Beaven

Keywords: easy-pickings added

comment:22 Changed 9 years ago by Jannis Leidel

Patch needs improvement: unset

Changed 9 years ago by Jannis Leidel

Attachment: add_default_to_for.4.diff added

fifth implementation with optional nodelist_default, including documentation and tests.

comment:23 Changed 8 years ago by Jannis Leidel

milestone: post-1.0
Status: newassigned

Changed 8 years ago by Jannis Leidel

Attachment: ticket6398-r9032.diff added

comment:25 Changed 8 years ago by Jacob

Resolution: fixed
Status: assignedclosed

(In [9530]) Fixed #6398: added an optional {% empty %} clause to the {% for %} template tag. The contents of this clause are rendered if the list iterated over turns out to be empty. Thanks, Jannis Leidel.

Astute readers will notice that the patch originally called this default; after consideration I decided that empty is a very slightly better color for this particular bikeshed.

comment:26 Changed 8 years ago by Stein Magnus Jodal

Resolution: fixed
Status: closedreopened

As far as I can see, the conversion from 'default' to 'empty' in the patch is not complete. At least, the tests seems broken, as they use "{% default %}" and not "{% empty %}".

Check out the following in #9530:

  • django/trunk/django/template/defaulttags.py lines 682 and 684 (and optionally 'nodelist_default' at 685, 688-689), and
  • django/trunk/tests/regressiontests/templates/tests.py lines 487-489.

comment:27 Changed 8 years ago by Russell Keith-Magee

Resolution: fixed
Status: reopenedclosed

This problem was fixed in [9532], which was committed a few hours after [9530].

comment:28 Changed 8 years ago by (none)

milestone: post-1.0

Milestone post-1.0 deleted

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