Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#13567 closed (fixed)

cycle tag with 'as' shouldn't return a value

Reported by: anentropic Owned by: lrekucki
Component: Template system Version: master
Severity: Keywords:
Cc: ego@…, lrekucki Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

if you use the 'as' form of the cycle tag, eg:

    {% cycle 'odd' 'even' as row_class %}

...that cycle tag still outputs a string at that point in the template.
Usually tags which set a context variable don't return a value - if you want to output the value you can do that separately with {{ row_class }}

It's unhelpful in the following use case:

    <table>
    {% for item in outer_loop %}
        {% cycle 'even' 'odd' as row_class %}
        {% for row in inner_loop %}
            <tr class="{{ row_class }}">
        {% endfor %}
    {% endfor %}
    </table>

i.e. you want to cycle values based on the outer loop, but they have to be output from within the inner loop. AFAIK you can't use the cycle tag within the inner loop as it'll pick up the wrong loop.

The code above works, but you also get 'odd even odd even odd even odd even...' appearing above the table, with no way to avoid it.

I think if you use the 'as' form it shouldn't output a value. Patch attached.

Attachments (4)

defaulttags.patch (2.3 KB) - added by anentropic 5 years ago.
backwards compatible with 'silent mode' flag
template.html (1.9 KB) - added by anentropic 5 years ago.
test file
urls.py (286 bytes) - added by anentropic 5 years ago.
test file
ticket13567__r13864.diff (4.8 KB) - added by lrekucki 5 years ago.
Patch with docs and tests.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 5 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

The issue here is backwards compatibility - while I agree with your analysis that when the 'as' tag is used, it shouldn't also output, the fact remains that the tag currently does output, and any existing use will be relying on this fact (or at least using it as currently implemented).

I suspect that this may need to be a Django 2.0 backwards incompatible change.

comment:2 Changed 5 years ago by anentropic

Here's a new patch so that cycle will accept a 'silent mode' flag as an arg after the variable name. eg

{% cycle 'row1' 'row2' 'row3' as rowcolors 1 %}

If the flag isn't present the tag behaves as before, if the flag evaluates True then it suppresses the output.

Once I got into doing it I discovered I think the reason why the tag was built so that the 'as' form still outputs a value. It's not explained very clearly in the docs, but when you use the 'by name' form:

{% cycle rowcolors %}

...you are re-using the actual cycle tag (with 'as') that you created earlier, and expecting it to output a value. I guess it's useful sometimes, but a bit confusing as this is different to how other tags work.

Anyway, with this new patch the cycle tag will work exactly as before, unless you specify the 'silent' flag. In that case the 'by name' form won't be useful but I guess people can live with that.

Changed 5 years ago by anentropic

backwards compatible with 'silent mode' flag

Changed 5 years ago by anentropic

test file

Changed 5 years ago by anentropic

test file

comment:3 Changed 5 years ago by cootetom

I've implemented this patch as it's something I needed since upgraded to 1.2. However I felt the syntax used to silence the cycle tag output wasn't very readable so I changed it slightly to that I could write {% cycle 'row1' 'row2' 'row3' as rowcolors silent %} in my templates instead of using the number 1 or 0!

Maybe something to consider if this patch get's placed into the main branch. Thanks for the patch!

comment:4 Changed 5 years ago by anentropic

That is more readable, maybe I prefer it.

When I wrote the patch I was thinking about making it easy to set the flag from a template variable. For most use cases you'd be hard-coding the silent flag for a particular cycle tag in the template anyway I guess.

Actually the way it was coded originally you could write your cycle tags like:

{% cycle 'row1' 'row2' 'row3' as rowcolors 'silent' %}

...since the string 'silent' will evaluate to True.

comment:5 Changed 5 years ago by russellm

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Design decision needed to Accepted

I'm going to accept this; Given the changes in 1.2 to the cycle tag, the need to be able to define a cycle without actually using it is important enough that we can't punt this to 2.0.

Some implementation notes:

  • Needs tests. The regressiontests/templates tests contain lots of examples; extending them to add extra cycle tests should be trivial
  • Needs docs. This is a new feature on the tag; docs aren't optional
  • The 'silent' keyword doesn't need to be quoted. It's in a known position, and there's no reason for it to be a template variable (that I can see).
  • The tag validation code has been made a lot more liberal in the provided patch. As *must* be the second last term, or the third last term if silent is used. Just checking for the location of "as" is loosening of validation.
  • If you use the cycle in silent mode, the definition shouldn't constitute a use of the cycle. i.e.,
    {% cycle 'row1' 'row2' 'row3' as rowcolor silent %}
    {% for val in range %}
        {% cycle rowcolor %}
    {% endfor %
    

should evaluate as row1 row2 row3 row1 ... , not row2 row3 row1 ... .

comment:6 Changed 5 years ago by lrekucki

  • milestone set to 1.3
  • Owner changed from nobody to lrekucki

Changed 5 years ago by lrekucki

Patch with docs and tests.

comment:7 Changed 5 years ago by lrekucki

  • Cc lrekucki added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I'm pretty sure the docs in the patch need some grammar review by some native speaker.

comment:8 Changed 4 years ago by russellm

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

(In [14439]) Fixed #13567 -- Added a 'silent' argument to the cycle tag, so that you can declare a cycle without producing a value in the template. Thanks to anentropic for the suggestion and initial patch, and Łukasz Rekucki for the final patch.

comment:9 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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