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 elseif case ("alias" of elif) to parseIf #826

Merged
merged 4 commits into from
Aug 31, 2016
Merged

Add elseif case ("alias" of elif) to parseIf #826

merged 4 commits into from
Aug 31, 2016

Conversation

kswedberg
Copy link
Contributor

This commit adds an elseif case that falls through to elif. Having elseif provides consistency between nunjucks and twig so the same templates can be used with either one.

@carljm
Copy link
Contributor

carljm commented Aug 31, 2016

Can you add a test for this?

@kswedberg
Copy link
Contributor Author

Sure, I'd be happy to do that. Any suggestions for what exactly to test? I was thinking I'd replicate an elif test, bit the only test I see for elif in /tests/parser.js is this one:

expect(function() {
    parser.parse('hello {% if sdf %} data');
}).to.throwException(/expected elif, else, or endif/);

@carljm
Copy link
Contributor

carljm commented Aug 31, 2016

Heh, looks like the existing testing of elif leaves something to be desired. I'd just add a compiler test (these are in practice our end-to-end functional tests) showing that if/elseif/endif works as expected. And if you're up for adding one for elif while you're at it, that'd be great :-)

@carljm
Copy link
Contributor

carljm commented Aug 31, 2016

(by "works as expected" I just mean something simple like {% if foo == 1 %}one{% elseif foo == 2 %}two{% endif %} and pass in a few different values for foo in the context, assert the template output.

@carljm
Copy link
Contributor

carljm commented Aug 31, 2016

Tests look great, thanks! And sorry I forgot to mention this in the first round, but would you also be willing to add a changelog entry (in the 2.x section), following the pattern of the existing entries? Thanks!

@kswedberg
Copy link
Contributor Author

Sure thing.

Also, not sure if you want docs updated or not, but I could change:

You can specify alternate conditions with elif and else

to:

You can specify alternate conditions with elif (or elseif, which is simply an alias of elif) and else

@carljm
Copy link
Contributor

carljm commented Aug 31, 2016

Sure, please do update the docs as well.

@kswedberg
Copy link
Contributor Author

Ok, I think I have everything in there, and I hope I didn't make too much of a mess out of the merge I just did. I thanked myself in the changelog, which was weird, but it fit the pattern, so what the heck.

@carljm
Copy link
Contributor

carljm commented Aug 31, 2016

Looks great, thank you!

@carljm carljm merged commit 5ee497c into mozilla:master Aug 31, 2016
carljm pushed a commit that referenced this pull request Aug 31, 2016
* Add elseif case ("alias" of elif) to parseIf

* Add elif/elseif compiler tests

* Update changelog and docs for elseif addition
@kswedberg kswedberg deleted the elseif-twig-sync branch September 1, 2016 14:34
@naterkane
Copy link

@carljm @kswedberg (Hi Karl, long time to talk!)
do either of you know if this has been released yet? I just stumbled on this PR as I was trying to see why elseif wasn't working.

I'm currently using

  • "nunjucks": "2.5.x"
  • "nunjucks-markdown": "2.0.x"

I'm not sure that I want to bump any dependencies at the moment.

@kswedberg
Copy link
Contributor Author

Hey @naterkane ! Hope you're well.
According to the commit history, elseif was included in the 2.5.0 release. (0ed3089#diff-48449cc0d8ca4858eddbb24677cf304eR3078). I've been using it successfully since then.

Are you sure you're using 2.5+? If you go into node_modules/nunjucks/package.json, what's the version property?

@naterkane
Copy link

yes, I'm currently using 2.5.2

"version": "2.5.2"

@kswedberg
Copy link
Contributor Author

Huh. that's strange. So, elif works, but elseif doesn't? if there's any way I can take a look at your setup, I'd be happy to help investigate

@naterkane
Copy link

naterkane commented Jan 7, 2017 via email

@kswedberg
Copy link
Contributor Author

ok, cool. I cloned the repo and npm installed. Changed elif to elseif in lib/resource.nunjucks and then ran node script from inside examples directory. No errors.

Is there some other way I should be testing this? Tried running npm run examples, but that didn't work for other reasons (both elif and elseif)

@naterkane
Copy link

naterkane commented Jan 7, 2017 via email

@kswedberg
Copy link
Contributor Author

ah, ok. I got the submodule in there and ran npm run examples, still with no errors. everything looks good. if there's somewhere in particular where you're seeing things break, more information would be helpful. so far, though, I can't seem to reproduce the problem.

@naterkane
Copy link

naterkane commented Jan 7, 2017 via email

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.

None yet

3 participants