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

Add failing test for passing vars from parent macro to child macro. #917

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jgerigmeyer
Copy link
Contributor

This PR simply adds a failing test for the most basic case of #912 (probably also related to #906).

@ArmorDarks
Copy link

I'm not sure that all those cases are allowed in Jinja2, Some older commits, like #667, clearly adding stronger isolation of child from parent scope. I guess to follow Jinja2 behavior better.

It's still unclear for me, does Jinja2 allows directly passing down variables to childs from parents, like in tests, or only via arguments, like in 3 and 4 test cases of #912.

Sorry @carljm for calling you once again, there isn't much Jinja2 experts around here :)

@jgerigmeyer
Copy link
Contributor Author

@ArmorDarks I tested all of the cases in your example #912; they all work as expected in Jinja2.

@carljm
Copy link
Contributor

carljm commented Nov 18, 2016

Don't need to be a Jinja2 expert to test its behavior :-) Usually for these edge cases (I've personally never nested a macro definition inside another macro in real uses) I don't know the behavior, I have to check it. Here are the scripts I use for comparing Jinja2 behavior to nunjucks behavior on rendering some template:

$ cat jinja.py 
#!/usr/bin/env python
import sys
from jinja2 import Environment, FileSystemLoader

env = Environment(loader=FileSystemLoader('.'), autoescape=False)
tpl = env.get_template(sys.argv[1])
print (tpl.render({}))

[09:24] carljm@radicchio:~/p/n/test 
$ cat nunjucks.js 
#!/usr/bin/env node
var nunjucks = require('../nunjucks/index.js');

var env = nunjucks.configure('.');
var output = env.render(process.argv[2], {});
console.log(output);

The first script requires that you first create a Python virtualenv, activate it, and then run pip install jinja2. The second requires that Node is installed, and that a checkout of nunjucks is present in ../nunjucks/ (personally I have these two scripts and a bunch of test templates in a directory adjacent to my nunjucks checkout).

@jgerigmeyer
Copy link
Contributor Author

(I didn't add separate test cases for each example, but I can if you'd like.)

@carljm I agree that the examples in #912 are somewhat edge cases. #906, however, isn't at all. I assume they're related, but I could add a test specifically for #906 if you'd prefer that.

@carljm
Copy link
Contributor

carljm commented Nov 18, 2016

I agree. They are quite probably related (IIRC effectively {% call %} defines a temporary un-named macro which is passed into the called macro as caller), although I think the codepaths are distinct enough from a regular macro definition that having a test specifically for that case is probably a good idea.

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.

3 participants