Django

Code

Ticket #6398 (closed: fixed)

Opened 11 months ago

Last modified 1 week ago

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

Reported by: jezdez Assigned to: jezdez
Milestone: post-1.0 Component: Template system
Version: SVN Keywords: easy-pickings
Cc: Triage Stage: Accepted
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

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

add_else_to_for.diff (3.8 kB) - added by jezdez on 01/16/08 17:10:45.
First implementation, including documentation
add_default_to_for.diff (3.8 kB) - added by jezdez on 01/17/08 12:11:13.
Second implementation, including documentation
add_default_to_for.2.diff (4.9 kB) - added by jezdez on 01/22/08 05:44:22.
Third implementation, including documentation and tests, fixing some issues, thanks SmileyChris?!
add_default_to_for.3.diff (5.6 kB) - added by jezdez on 01/23/08 06:02:09.
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 jezdez on 03/20/08 12:50:38.
fifth implementation with optional nodelist_default, including documentation and tests.
ticket6398-r9032.diff (5.4 kB) - added by jezdez on 09/15/08 06:25:13.
Updated patch to Django r9032 (http://github.com/jezdez/django/tree/ticket6398)

Change History

01/16/08 17:10:45 changed by jezdez

  • attachment add_else_to_for.diff added.

First implementation, including documentation

01/16/08 18:23:21 changed by Simon Greenhill <dev@simon.net.nz>

  • needs_better_patch changed.
  • stage changed from Unreviewed to Ready for checkin.
  • needs_tests changed.
  • needs_docs changed.

01/16/08 18:58:14 changed by Rob Hudson <treborhudson@gmail.com>

The same be accomplished by this:

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

01/16/08 19:21:12 changed by anonymous

  • stage changed from Ready for checkin to Design decision needed.

01/16/08 20:51:21 changed 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.

01/16/08 22:49:35 changed by adrian

  • stage changed from Design decision needed to Accepted.

The idea gets a +1 from me.

01/17/08 07:29:18 changed by Matthias Pronk <django@masida.nl>

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).

01/17/08 07:57:02 changed by jezdez

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.

01/17/08 09:07:14 changed 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.

01/17/08 09:07:35 changed by Simon Willison

01/17/08 12:11:13 changed by jezdez

  • attachment add_default_to_for.diff added.

Second implementation, including documentation

01/17/08 12:12:45 changed by jezdez

  • summary changed from Add optional {% else %} clause to {% for %} template tag to Add optional {% default %} clause to {% for %} template tag.

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

01/17/08 12:14:54 changed by jezdez

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!'

01/21/08 22:10:18 changed by SmileyChris

  • needs_better_patch set to 1.
  • needs_tests set to 1.

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.

01/22/08 05:44:22 changed by jezdez

  • attachment add_default_to_for.2.diff added.

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

01/22/08 05:45:39 changed by jezdez

  • needs_tests deleted.

01/22/08 05:46:12 changed by jezdez

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

01/22/08 11:58:14 changed by SmileyChris

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**"

01/22/08 12:23:27 changed by jezdez

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?
    ...

01/22/08 14:21:21 changed by SmileyChris

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

01/23/08 06:02:09 changed by jezdez

  • 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 :/

01/23/08 06:03:22 changed by jezdez

Is the last patch what you meant?

01/23/08 12:05:57 changed by SmileyChris

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

02/06/08 06:09:45 changed by jezdez

  • owner changed from nobody to jezdez.

02/21/08 15:31:45 changed by SmileyChris

  • keywords set to easy-pickings.

03/18/08 14:50:53 changed by jezdez

  • needs_better_patch deleted.

03/20/08 12:50:38 changed by jezdez

  • attachment add_default_to_for.4.diff added.

fifth implementation with optional nodelist_default, including documentation and tests.

06/25/08 12:26:16 changed by jezdez

  • status changed from new to assigned.
  • milestone set to post-1.0.

09/15/08 06:25:13 changed by jezdez

  • attachment ticket6398-r9032.diff added.

11/24/08 16:01:50 changed by jacob

  • status changed from assigned to closed.
  • resolution set to fixed.

(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.

11/26/08 03:26:00 changed by jodal

  • status changed from closed to reopened.
  • resolution deleted.

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.

11/26/08 03:44:25 changed by russellm

  • status changed from reopened to closed.
  • resolution set to fixed.

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


Add/Change #6398 (Add optional {% default %} clause to {% for %} template tag)




Change Properties
Action