Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

regroup tag not 100% dtl compatible #101

Closed
kaos opened this Issue Nov 30, 2013 · 18 comments

Comments

Projects
None yet
2 participants
Owner

kaos commented Nov 30, 2013

The erlydtl version of the regroup tag requires endregroup, which Django does not.

Member

seriyps commented Feb 10, 2014

How do you want to inject new variables in global lookup dictionary? I mean

{% regroup cities by country as country_list %}

this experssion injects country_list variable to global (or current??) context, so it can be accessed like, eg {{country_list}} or so. But I can't find any example how this may be implemented in erlydtl_compiler.

This is also relevant for {% url ... as var %} and {% trans "smth" as var %}, maybe somewhere else.

Owner

kaos commented Feb 10, 2014

I'm not sure how it works in Django, but I don't expect it to be injected in the global scope. Only change is that it won't have to introduce a new local scope, but simply add to the current scope. That way we don't need to explicitly tell when the regroup scope ends, as it's relying on the surrounding scope.

There's a local_scope list in the #dtl_context{} state passed around the compiler for this.

Thanks for the heads up on the url and trans tags. Will look into those.

Member

seriyps commented Feb 10, 2014

The problem is that local_scope isn't persisted between the steps of mapfoldl https://github.com/erlydtl/erlydtl/blob/0.8.2/src/erlydtl_compiler.erl#L946
So, it require some not-so-trivial rewrite, that's what I want to say.

Owner

kaos commented Feb 10, 2014

Ah, ok. I haven't looked at how to implement this. Yet.
Den 10 feb 2014 17:49 skrev "Sergey Prokhorov" notifications@github.com:

The problem is that local_scope isn't persisted between the steps of
mapfoldl
https://github.com/erlydtl/erlydtl/blob/0.8.2/src/erlydtl_compiler.erl#L946
So, it require some not-so-trivial rewrite, that's what I want to say.

Reply to this email directly or view it on GitHubhttps://github.com/erlydtl/erlydtl/issues/101#issuecomment-34653329
.

Owner

kaos commented Feb 11, 2014

Well, I don't see why you'd need the local_scope persisted in that mapfoldl to begin with.

As the body_ast is called recursively for nested child scopes..

Member

seriyps commented Feb 11, 2014

But how you can make as value available down the definition, if not introduce new endless local scope or make current local_scope persistent?

Say, you have template like

<html>
{% regroup a by b as c%} {# maproldl step - add 'c' to current local_scope #}

{{c}} {# mapfoldl next step, since 'local_scope' didn't persisted between mapfoldl steps, and no new local scope introduced, 'c' isn't available there #}

{% for x in c %} {# the same there #}
   {# smth #}
{% endfor %}


</html>
{# EOF #}

the parser's AST for it will be smth like

[{regroup, {.., .., {identifier, c}} },
 {variable, {identified, c}},
 {for ...}].

So, looks like there should be some mutable state between mapfoldl steps and smth like mutable 'memo object' in generated code (note, that defined names may be re-defined later).

Owner

kaos commented Feb 24, 2014

You're right, we need the context to be mutable while traversing the parse tree, pretty much as #treewalker{} is now.. which brings me to how I intend to solve this. Namely to tag the context along with the tree walker while traversing the tree. That way, the context can be updated at each step (along with the tree walker).

And as you suspected, this is a non-trivial change (not complicated, just a lot of minor edits).

kaos added a commit that referenced this issue Feb 26, 2014

Make endregroup tag optional (fixes #101)
Django does not have endregroup at all.
We keep it as optional for backward compatibility with ErlyDTL.

The dtl context has been moved into the treewalker record while
traversing the template AST, to allow it to change on any node.

Local scopes can now begin at any point in a block, and all
nested scopes are automatically closed when the block scope ends.
Owner

kaos commented Feb 26, 2014

Ok, it got a little more complicated than I hoped, when I realized that I had to introduce support for nested sub scopes in the compiled template..

But it works now. The regroup tag introduce new variable binding when declared, and are valid until the end of the surrounding block.

@kaos kaos modified the milestones: 0.9.1, 1.0 Feb 26, 2014

@kaos kaos self-assigned this Feb 26, 2014

@kaos kaos added the fixed label Feb 26, 2014

Member

seriyps commented Feb 26, 2014

Wow, great work!
Now we can apply the same technique for trans tag.

Owner

kaos commented Feb 26, 2014

Yep, and for the upcoming url tag :)
Do you know of any others that can have as .. annotations?
It should be fairly easy to implement a generic support in the compiler for adding the as .. scope now, but the parser may be trickier to get to support it generally.. guess we'll simply have to add rules for each tag that need to support it..

Member

seriyps commented Feb 26, 2014

Others... Maybe 'cycle' and 'static'. Not sure, I just searched Django docs.

2014-02-26 23:01 GMT+04:00 Andreas Stenius notifications@github.com:

Yep, and for the upcoming url tag :)
Do you know of any others that can have as .. annotations?
It should be fairly easy to implement a generic support in the compiler
for adding the as .. scope now, but the parser may be trickier to get to
support it generally.. guess we'll simply have to add rules for each tag
that need to support it..

Reply to this email directly or view it on GitHubhttps://github.com/erlydtl/erlydtl/issues/101#issuecomment-36163054
.

Owner

kaos commented Feb 26, 2014

Ah, ok.
I'm not fluent in Django either.. relying on the docs ;)

Have vague ideas about having a test suite comparing output results when compiled with ErlyDTL and different Django versions.. but that is probably far far away..

Member

seriyps commented Feb 26, 2014

having different Django versions is relatively easy using virtualenv, like

$ virtualenv 'with_django1.4'
$ source with_django1.4/bin/activate
$ pip install django==1.4
$ python -c 'do smth with django 1.4'
$ deactivate
$ virtualenv 'with_django1.6'
$ source with_django1.6/bin/activate
$ pip install django==1.6
$ python -c 'do smth with django 1.6'
...
Owner

kaos commented Feb 26, 2014

Yeah, I've seen that it would be doable (also there are tests in django that compares different versions for some reason..) the "hard" (or maybe rather just tedious) part would be to setup and run a test suite comparing the two template engines.. (but then again, for me it might be hard as I'm not a python guy either)

kaos added a commit that referenced this issue Feb 26, 2014

Fix bug in new scope routine (#101)
When there was some text surrounding the sub scope,
it only kept the last piece.
Tests updated to catch this.
Owner

kaos commented Feb 26, 2014

trans .. as .. fixed in 4aa8787, which also shows how easy it was ;)

Member

seriyps commented Feb 27, 2014

the "hard" (or maybe rather just tedious) part would be to setup and run a test suite comparing the two template engines.. (but then again, for me it might be hard as I'm not a python guy either)

Not so hard, everything that needed is my virtualenv snippet. But I think it's better to do after #134 will be done (so, all this virtualenv stuff may be installed to .eunit folder, which eunit create anyway).

trans .. as .. fixed in 4aa8787, which also shows how easy it was ;)

That's cool!

Owner

kaos commented Feb 27, 2014

I invite you to do the virtualenv python django test stuff when #134 has been closed ;)

@kaos kaos closed this Feb 28, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment