#13567 closed (fixed)
cycle tag with 'as' shouldn't return a value
Reported by: | Paul Garner | Owned by: | Łukasz Rekucki |
---|---|---|---|
Component: | Template system | Version: | dev |
Severity: | Keywords: | ||
Cc: | ego@…, Łukasz Rekucki | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
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)
Change History (13)
comment:1 by , 14 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
comment:2 by , 14 years ago
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.
comment:3 by , 14 years ago
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 by , 14 years ago
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 by , 14 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
Triage Stage: | Design decision needed → 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 by , 14 years ago
milestone: | → 1.3 |
---|---|
Owner: | changed from | to
comment:7 by , 14 years ago
Cc: | 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 by , 14 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
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.