Opened 8 years ago

Closed 5 years ago

#4552 closed (fixed)

Tidy up "for" tag

Reported by: SmileyChris Owned by: nobody
Component: Template system Version: master
Severity: Keywords: for loop
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Just a follow-up patch related to #3523 (Extend "for" tag to allow unpacking of lists) and it's recent checkin.

Currently there is a small difference between the single behaviour and the unpacking behaviour: for unpacking, the context scope is popped every loop iteration. This patch changes this behaviour to use the same scope throughout the loop's lifespan (making it behave more like the older single behaviour).

The patch also tidies up the comma-splitting through the use of re.split.

Attachments (2)

for_tag_tidy.patch (2.3 KB) - added by SmileyChris 8 years ago.
for_tag_re_split.patch (674 bytes) - added by anonymous 5 years ago.

Download all attachments as: .zip

Change History (9)

Changed 8 years ago by SmileyChris

comment:1 Changed 8 years ago by SmileyChris

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

comment:2 Changed 8 years ago by jacob

Hrm... this is a tricky one. The re.split change is obviously good, but the other one looks like it'll be a bit slower, which I don't like.

comment:3 Changed 6 years ago by SmileyChris

  • Needs tests set

Going back through some of my old tickets.

If anything, I'd say this would be a speed increase. Currently, we're pushing and popping the context like crazy for unpacked fors. The second for is a safety method which is mostly a noop (except for a len() call) for most normal usage.

Apart from that, it is also a subtle change in behaviour (new items placed on the context are local to each iteration unpacked fors).

comment:4 Changed 5 years ago by SmileyChris

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

This is much less important now we have a RenderContext which should be used if you really care about context. Due to the age of the ticket, switching back to the original behavior would potentially cause more side-effects in third party projects than leaving it as-is.

The ticket is being left open because as jacob says the split change is good. So the "improved" patch just needs to do that.

Changed 5 years ago by anonymous

comment:5 Changed 5 years ago by anonymous

  • Triage Stage changed from Accepted to Ready for checkin

comment:6 Changed 5 years ago by anonymous

  • Patch needs improvement unset

comment:7 Changed 5 years ago by SmileyChris

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

(In [14656]) Fixed #4552 -- minor tidy up of the {% for %} tag's comma splitting

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