Opened 16 years ago

Last modified 3 months ago

#5865 new New feature

cycle template tag should accept a single argument

Reported by: Gary Wilson Owned by:
Component: Template system Version: dev
Severity: Normal Keywords:
Cc: mateusz@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When passed a single argument, the cycle tag should treat it as an iterable to cycle through. For example, with the context:

{'colors': ['red', 'blue', 'green']}

you should be able to have a template like:

{% for row in mydata %}
<tr class="{% cycle colors %}">...</tr>
{% endfor %}

that would output:

<tr class="red">...</tr>
<tr class="blue">...</tr>
<tr class="green">...</tr>
<tr class="red">...</tr>
...

Attachments (2)

cycle.diff (6.7 KB ) - added by munhitsu 16 years ago.
v2
cycle-doc.diff (1.2 KB ) - added by munhitsu 16 years ago.

Download all attachments as: .zip

Change History (28)

comment:1 by Simon G <dev@…>, 16 years ago

Triage Stage: UnreviewedAccepted

This sounds like a sensible improvement

comment:2 by munhitsu, 16 years ago

Owner: changed from nobody to munhitsu

comment:3 by Malcolm Tredinnick, 16 years ago

I'm a little concerned that this is ambiguous when you're reading it: you can't easily tell if it's referring to a previously named "cycle" tag or creating a new one using a variable as an iterator.

Is there a better way to write this that won't cause confusion when reading templates?

comment:4 by munhitsu, 16 years ago

So far I've managed to perform the request but maintaining compatibility with older approach makes my code not so... clean.

What do you think about a new keyword i.e.: cycletable?

Maybe its a time to refer to named cycle tag by variable reference? To be honest referring by name is not to clear for me.

in reply to:  3 comment:5 by Gary Wilson, 16 years ago

Replying to mtredinnick:

I'm a little concerned that this is ambiguous when you're reading it: you can't easily tell if it's referring to a previously named "cycle" tag or creating a new one using a variable as an iterator.

Is it really that different from using the "as varname" syntax of other tags or the {% with %} tag though? The way I see it, when you encounter any variable name in the templates, you have to check where it came from. Was it passed in the context? Was it defined as the target of "as varname" syntax of another variable? Was it defined with a with tag? Is it a loop variable? etc. This would be no different.

comment:6 by munhitsu, 16 years ago

Cc: mateusz@… added

Guys take a look at following unit test suite. Does it reflect your expectations?

{
'cycle20': ("{% cycle colors %}", {'colors': 'red'}, template.TemplateSyntaxError),
'cycle21': ("{% cycle colors %}{% cycle colors %}", {'colors': ['red', 'blue', 'green']}, 'redblue'),
'cycle22': ("{% cycle colors %}{% cycle colors %}{% cycle colors %}", {'colors': ['red', 'blue', 'green']}, 'redbluegreen'),
'cycle23': ("{% cycle colors %}{% cycle colors %}{% cycle colors %}{% cycle colors %}", {'colors': ['red', 'blue', 'green']}, 'redbluegreenred'),
'cycle24': ("{% cycle colors as color %}{% cycle color %}", {'colors': ['red', 'blue', 'green']}, 'redblue'),
'cycle25': ("{% cycle colors as color %}{% cycle color %}{% cycle color %}", {'colors': ['red', 'blue', 'green']}, 'redbluegreen'),
'cycle26': ("{% cycle colors as color %}{% cycle color %}{% cycle color %}{% cycle color %}", {'colors': ['red', 'blue', 'green']}, 'redbluegreenred'),
'cycle27': ("{% for i in test %}{% cycle colors %}{{ i }},{% endfor %}", {'test': range(5),'colors': ['a', 'b']}, 'a0,b1,a2,b3,a4,'),
}

comment:7 by munhitsu, 16 years ago

Has patch: set
Resolution: fixed
Status: newclosed

Seems to be working. Old regression is passing. New has been added.
I've added a small enhancement - a cycle pushed to context. Thanks to it cycle internals can be used inside template.

        context['cycle'] = {
            # cycle internals
            'collection': self.collection,
            'name':self.variable_name,
            'value':value,
        }

One note. Last part of syntax parsing has to be done in render method. We need context to say if {% cycle colors %} refers to table or is a mistake.

comment:8 by munhitsu, 16 years ago

Resolution: fixed
Status: closedreopened

comment:9 by Gary Wilson, 16 years ago

Needs documentation: set
Patch needs improvement: set

Your cycle20 test case should not raise a TemplateSyntaxError, but rather should output "r" since a string is iterable. A TemplateSyntaxError should only be raised if the syntax for the tag in not correct, i.e. {% cycle colors as %}. If the object passed is not iterable, the template tag should fail silently and return an empty string. Also, your patch should allow any iterable instead of just lists. I also don't see a reason that we should be exposing the internal state of the tag. Could you explain your reasoning for wanting to do that?

by munhitsu, 16 years ago

Attachment: cycle.diff added

v2

comment:10 by munhitsu, 16 years ago

Patch needs improvement: unset

Replying to gwilson:

Thanks for the comments.

  1. Functionality for test cycle20 updated. Do you have an idea how to prepare context with non iterable variable? I have no clue how to make unit test for it.
    'cycle20': ("{% cycle colors %}", {'colors': 'r'}, 'r')
    
  2. Regarding silent fail... Actual implementation will throw TemplateSyntaxError for test cycle01 as original one. But i.e if 'a' would be defined in context than it will fall back to Variable('a').resolve(context).
    'cycle01': ('{% cycle a %}', {}, template.TemplateSyntaxError)
    
  3. I've just added NameError exception catch as a backup to make sure we will fail silently
  4. Cycle patch now iterates over everything that is iterable - see test cycle22. It's hashed as Python does not guarantee iteration sequence on hash.
    'cycle22': ("{% cycle colors %}{% cycle colors %}", {'colors': {'r': 1, 'g': 2}}, 'rg')
    
  5. Regarding cycle internal state in tag. I must say that I was inspired by ForNode and cycle is a somehow close to it. Take a look at tests cycle28..29. That the only reasonable case that I see now to use cycle internals. It's your choice.
    'cycle28': ("{% for i in test %}{% cycle colors %}{{ cycle.value }}{{ i }},{% endfor %}", {'test': range(5),'colors': ['r', 'g', 'b']}, 'rr0,gg1,bb2,rr3,gg4,'),
    'cycle29': ("{% for i in test %}{% cycle colors %}{% cycle colors %}{{ i }},{% endfor %}", {'test': range(5),'colors': ['r', 'g', 'b']}, 'rg0,br1,gb2,rg3,br4,'),
    

by munhitsu, 16 years ago

Attachment: cycle-doc.diff added

comment:11 by munhitsu, 16 years ago

Cc: mateusz@… added; mateusz@… removed
Needs documentation: unset

comment:12 by Gabriel Hurley, 13 years ago

Severity: Normal
Type: New feature

comment:13 by Julien Phalip, 13 years ago

Easy pickings: unset
Triage Stage: AcceptedDesign decision needed

A design decision hasn't been reached yet.

comment:14 by Alex Gaynor, 13 years ago

Triage Stage: Design decision neededAccepted
UI/UX: unset

Discussion with Carl: this new syntax would be ambiguous with the current syntax, however this is a good feature, and the current API is kind of silly. This is accepted, provisional on creating a new, more sane and consistent syntax, to be placed in the future module.

comment:15 by Preston Holmes, 13 years ago

Needs documentation: set
Needs tests: set
Patch needs improvement: set

comment:16 by Aymeric Augustin, 11 years ago

Status: reopenednew

comment:17 by Alexander Lazarević, 3 months ago

Is this still considered a desirable feature?

in reply to:  17 comment:18 by Mariusz Felisiak, 3 months ago

Replying to Alexander Lazarević:

Is this still considered a desirable feature?

Yes, IMO nothing has changed.

comment:19 by Alexander Lazarević, 3 months ago

Owner: changed from munhitsu to Alexander Lazarević
Status: newassigned

comment:21 by Alexander Lazarević, 3 months ago

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

For the new PR I'm going to remove "Needs documentation", "Needs tests" and "Patch needs improvement". Please set these flags again if required (or I should have left them set in the first place).

comment:22 by Alexander Lazarević, 3 months ago

I just put something together at https://sandbox.e11bits.com/ to showcase the feature more broadly. Maybe people can judge by that, if the feature is desirable and help with the code review? It's just a quick hack, so don't expect too much. The examples are more or less the ones found as testcases in the PR. (crossposted to discord)

in reply to:  22 comment:23 by Natalia Bidart, 3 months ago

Replying to Alexander Lazarević:

I just put something together at https://sandbox.e11bits.com/ to showcase the feature more broadly. Maybe people can judge by that, if the feature is desirable and help with the code review? It's just a quick hack, so don't expect too much. The examples are more or less the ones found as testcases in the PR. (crossposted to discord)

Thank you Alexander for your work on this. In order to unify the conversation in a single place, I will reply to the forum thread with my view, hopefully we can agree on an implementation design there and then come back to the ticket.

As a reference, this is the forum post.

comment:24 by Alexander Lazarević, 3 months ago

After the discussion in https://forum.djangoproject.com/t/cycle-template-tag-should-accept-a-single-argument/27010/3 I'm withdrawing the PR.

My implementation would have introduced an ambiguity that would have made it harder for users to use the cycle tag.

In the discussion @carlton mentioned that it could be worth "... to investigate [is] whether we could add * and *'* list and dictionary expansion to the DTL. cycle could benefit from list expansion.".

So I'm trying to come up with something in that direction instead.

Last edited 3 months ago by Alexander Lazarević (previous) (diff)

comment:25 by Mariusz Felisiak, 3 months ago

Has patch: unset

There is nothing to review, at least for now.

comment:26 by Alexander Lazarević, 3 months ago

Owner: Alexander Lazarević removed
Status: assignednew
Note: See TracTickets for help on using tickets.
Back to Top