-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
2.9 regression when assigning a variable inside a loop #641
Comments
While the current behavior is incorrect, the old behavior is definitely incorrect as well. The |
The behavior I would assume were correct is an output like |
In this specific case I resolved it by rewriting it to use |
I have the feeling this is a somewhat common use case - also stuff like |
I'm shocked this worked before. Did this always work? |
apparently yes:
|
Great. Because the issue here is that this is absolutely not sound. Which variable is it supposed to override? What if there is a function scope in between. eg: a macro or something like that. I have no idea how to support this now. |
hmm maybe similar to python's scoping assuming no |
Apparently before this was caught down with a template assertion error: >>> Template('{% set x = 0 %}{% for y in [1, 2, 3] recursive %}{{ x }}{% set x = y %}{% endfor %}').render()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
jinja2.exceptions.TemplateAssertionError: It's not possible to set and access variables derived from an outer scope! (affects: x (line 1) |
the behavior seems to be different with/without recursive (2.8.1 gives me an UnboundLocalError error with recursive and '012' without) |
The unbound local error should be resolved on master. |
I'm not sure how to progress here. I really dislike that it was apparently possible to do this sort of thing. |
While technically this applies to any scope and not just for loops it comes up most commonly in the context of for loops. This now defines the behavior for scoping in a way that is consistent but different than it was in the past. There is an ongoing conversation if we should keep it that way or not. References #641
With the last changes I'm going to close this as "works as intended". If there is further fallout from this we can investigate alternatives again. |
Variables defined in an outer scope can no longer be set from an inner scope (see pallets/jinja#641). Regardless of whether that is right or wrong, we can't control if people are using such constructs in their plugins, which versions of Jinja >= 2.9 would now break out of the blue, regardless of OctoPrint version. That is unacceptable sadly and requires pinning for now, until plugin authors have had a chance to adapt accordingly. Also see #1697.
Got sent to this issue (see #656) after this change blew up my blog template on upgrading from v.2.8.1 to v2.9.4. I was using it keep track if various pieces of data were changing between loop iteration. I was able to fix it because I wrote the original template code (see MinchinWeb/seafoam@8eb7608 and MinchinWeb/seafoam@89d555d), but I doubt I would have been able to otherwise. The new code is harder to follow as the comparisons are now done in-line. For example, my old code (v.2.8.1): {%- set last_day = None -%}
{% for article in dates %}
{# ... #}
<div class="archives-date">
{%- if last_day != article.date.day %}
{{ article.date | strftime('%a %-d') }}
{% else -%}
—
{%- endif -%}
</div>
{%- set last_day = article.date.day %}
{% endfor %} and the new code, with in-line comparisons (v2.9.4): {% for article in dates %}
{# ... #}
<div class="archives-date">
{%- if ((article.date.day == dates[loop.index0 - 1].date.day) and
(article.date.month == dates[loop.index0 - 1].date.month) and
(article.date.year == dates[loop.index0 - 1].date.year)) %}
—
{% else -%}
{{ article.date | strftime('%a %-d') }}
{%- endif -%}
</div>
{% endfor %} So I just wanted to say that the "feature" (or "hack", if you prefer) is used and is already missed. If the scoping issues are too complex to figure out sensibly at the moment, could something (at a minimum) be added to the changelog so it bites less people unaware? |
I was not aware this is so widely abused :-/ Annoyingly there is really no way to make this work in any reliable way. However I wonder if we can isolate the common use cases here and introduce a nicer api. In particular maybe we want to add something like this ( {% for article in dates %}
<div class="archives-date">
{%- if loop.changed_from_last(article.date.day) %}
{{ article.date | strftime('%a %-d') }}
{% else -%}
—
{%- endif -%}
</div>
{% endfor %} |
I guess the first element would always be considered "changed" no matter what it is? If that behavior is not OK for someone they could always add a |
Maybe |
just .....and now I can already imagine someone asking for a |
Or maybe an explicit statement for writing outside scope: |
i was thinking this: class LoopContextBase(object):
def __init__(self, recurse=None, depth0=0):
...
self._last_iteration = missing
def changed(self, *value):
if self._last_iteration != value:
self._last_iteration = value
return True
return False |
@davidism we cannot do |
Yeah, figured that would be the case. I'm anticipating "what if I want to know if the current value is greater than the previous", or something similar. But I like the |
@davidism I played around with loops in new jinja 2.9*. with 2.9* even when I create/define a new variable inside a loop it gets cleared/deleted at end of every iteration. {% for name in names %}
{% if loop.first %}
{% set list = "" %}
{% endif %}
{% if name.first is defined and name.last is defined and not name.disabled %}
{% set list = list + name.first|string + "-" + name.last|string %}
{% if loop.last %}
{% for item in list.split(' ')|unique %}
{{ item }}
{% endfor %}
{% else %}
{% set list = list + " " %}{% endif %}
{% endif %}
{% endfor %} This might not be the best way to do this but here from my understanding I am not breaking any scoping rules. |
Had this issue in 2.8 and superior Here goes a test case:
|
Use |
I think providing access to the previous loop value through an attribute of the {% set previous_date = none %}
{% for item in entries -%}
{% set date = item.start_dt.astimezone(tz_object).date() %}
{% if previous_date and previous_date != date -%}
...
{% endif %}
{% set previous_date = date %}
{%- endfor %} |
Yeah that sounds like an idea. |
What about adding |
Had the same idea, but then couldn't find any non-ugly cases where this would be needed. And I could already see someone asking for |
@davidism @ThiefMaster i was thinking of just having a storage object available. Like this: {% set ns = namespace() %}
{% set ns.value = 42 %}
...{% set ns.value = 23 %} Obviously set can't set attributes at the moment but this could easily be extended. Since no reassignment to the namespace itself happens this is quite safe to implement. |
Seems fine to me, it's the same solution as |
What I dislike with the namespace is that it'll feel very tempting to do Otherwise it looks like an interesting idea, even though I think |
@ThiefMaster it would not work. The way I would see this working is that attribute assignments go through a callback on the environment which would only permit modifications to namespace objects. |
ok, so at least no risk of templates causing unexpected side-effects on objects passed to them. still a bit wary of the things people might do...
Actually, I think a new block like |
See pallets/jinja#641 Signed-off-by: Rob Caelers <rob.caelers@gmail.com>
Do you have a workaround for this issue or do we have to wait for previtem/nextitem in 2.9.6? |
As has been demonstrated to varying degrees above, you might not even need to do what you're doing. Otherwise, yes, you need to wait if you want to use 2.9. It was never supported before, it just happened to work. |
…pt; changed some styles and got around that Jinja2 problem pallets/jinja#641
We will not be reverting to the old behavior. While it worked in simple cases, it was not correct and was never documented as supported. While I understand that it is a breaking change, it occurred in a feature release (second number change) which is how we've always managed these changes. Pin the version until a fix is released if you need to keep relying on the old behavior. Locking this because everything that needs to be said has been said. See #676 and #684 for the fixes that are currently being considered. |
2.9:
2.8:
Originally reported on IRC:
The text was updated successfully, but these errors were encountered: