Skip to content
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

extended context to enable implementation of swiftlike scoping rules … #198

Closed
wants to merge 1 commit into from

Conversation

kaygro
Copy link

@kaygro kaygro commented Feb 11, 2018

…(pushLocals function)

added a testcase for this new behavior
modified ForTag to make use of this new mechanism

…(pushLocals function)

added a testcase for this new behavior
modified ForTag to make use of this new mechanism
@kylef
Copy link
Collaborator

kylef commented Feb 11, 2018

Hi @kaygro, I'm having a hard time trying to understand what you are trying to solve that is different from the push method on Context which already takes a closure. Could you perhaps elaborate and perhaps provided an example use-case where the regular push method does not suffice?

@kaygro
Copy link
Author

kaygro commented Feb 12, 2018

If you want to to implement an extension, which has a set tag (as StencilSwiftKit for example),
you want to be able to do mutations inside a for loop, which are visible outside of it.
A resonable implementation of a set tag would do something like
context[variableName] = value,
but if that gets executed while rendering tags inside a for tag, it gets popped off afterwards when using regular push.

@kaygro
Copy link
Author

kaygro commented Feb 13, 2018

Exampe Stencil code for this scenario would look like:

{% set foundElem %}false{% endset %}
{% for elem in array %}
  {% if satisfiesInterestingProperty elem %}
    {% set foundElem %}true{% endset %}
  {% endif %}
{% endfor %}
{# When normal push/pop is used, foundElem will always be "false" at this point #}
{% if foundElem == "true" %}
...
{% endif %}

I'd like to stress, that this modification does not implement a set tag, but merely enables implementation of a set tag, that works like this.

@ilyapuchka
Copy link
Collaborator

Not sure about this change, feels too invasive, also I think it can be implemented in a simpler way, but I didn't explore it enough to be sure. Also I think current behaviour is the same as in Jinja.
Speaking of example shared, I believe it can be achieved by using filter on array and then checking its count.

{{ if array|yourCustomFilterThatChecksForPredicate|count > 0 }}

Of course you'll need to implement this filter and register it in environment, so there is no way to do it just with templates.
Another way would be to use filter that I've implemented in #189

@kaygro
Copy link
Author

kaygro commented Mar 4, 2018

Well, I think it can be implemented entriely from within the set node, but that would mean, the implementation of the set node would need to make copies at the various heights of the context stack, where that variable is visible, (for every variable change that is). It would be less cubersome, if we could observe/override the subscript operators of the Context class, but that is not possible due to access beeing public, not open and Context not being a protocol.

@djbe
Copy link
Contributor

djbe commented Jul 11, 2018

@kaygro I get what this is trying to achieve (seeing the issues with set), but I agree with the others that this might not be the best way to achieve this.

It might be better to have a mechanism in Context to directly modify the stack, so that set (for example) could modify a variable outside it's scope. For example modify the subscript operator here:
https://github.com/stencilproject/Stencil/blob/master/Sources/Context.swift#L17
To accept a parameter to, instead of modifying the current level, search through the levels and modify the topmost one it finds (or throw an error if it doesn't find it?).

@kaygro
Copy link
Author

kaygro commented Jul 13, 2018

I like that idea. I didn't realize, you could have multiple parameters in subscript operators.

@ilyapuchka
Copy link
Collaborator

Closing this due to inactivity.

@ilyapuchka ilyapuchka closed this Dec 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants