adds PyList support to ForTag#390
Conversation
boulter
left a comment
boulter
left a comment
There was a problem hiding this comment.
Looks good. I'd like to see a few more tests to confirm it's correct.
Thanks!
| @Test | ||
| public void testTuplesWithPyList() { | ||
| String template = "{% for href, caption in [('index.html', 'Index'), ('downloads.html', 'Downloads'), ('products.html', 'Products')] %}" + | ||
| "<li><a href=\"{{href|e}}\">{{caption|e}}</a></li>\n" + |
There was a problem hiding this comment.
any reason for running this through the escape filter (e) here?
There was a problem hiding this comment.
this was mentioned in issue #321, therefore, I used the exact same thing to test the functionality out. there is no specific reason for choosing this template. 😅
There was a problem hiding this comment.
Ok, can you remove anything that's not relevant to the test then? We don't want to be testing the escape filter here.
| "<li><a href=\"downloads.html\">Downloads</a></li>\n" + | ||
| "<li><a href=\"products.html\">Products</a></li>\n"; | ||
|
|
||
| String rendered = jinjava.render(template, context); |
There was a problem hiding this comment.
this could be broken out into a separate test
| expected = "<p>1 2 3</p>\n" | ||
| + "<p>4 5 6</p>\n"; | ||
| rendered = jinjava.render(template, context); | ||
| assertEquals(rendered, expected); |
There was a problem hiding this comment.
could you also add a test that confirms that this works for non-string values? Perhaps add 1 + 2 + 3?
There was a problem hiding this comment.
I've added tests for the same.
Also, I've modified the ForTag.java file to accommodate the changes.
| String entryVal = null; | ||
| // safety check for size | ||
| if (entries.size() >= loopVarIndex) { | ||
| entryVal = Objects.toString(entries.get(loopVarIndex)); |
There was a problem hiding this comment.
can you add a test for this case?
There was a problem hiding this comment.
This is a safety check only but I've added tests where the jinjava.render is expected to throw an exception when there are lesses values to unpack. 👍
| @Test | ||
| public void testTuplesWithPyList() { | ||
| String template = "{% for href, caption in [('index.html', 'Index'), ('downloads.html', 'Downloads'), ('products.html', 'Products')] %}" + | ||
| "<li><a href=\"{{href|e}}\">{{caption|e}}</a></li>\n" + |
There was a problem hiding this comment.
Ok, can you remove anything that's not relevant to the test then? We don't want to be testing the escape filter here.
2 participants
Fixes #321
This pull request is wrt the above-mentioned Issue.
The support for For-Tag was limited to Maps and the else block in ForTag.java file could not handle tuples(list of tuples) types. This PR adds the needed code as well as the supporting tests.
Cheers!