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

Bind 'in' operator more tightly than boolean operators. #336

Merged
merged 1 commit into from
Jan 8, 2015

Conversation

carljm
Copy link
Contributor

@carljm carljm commented Dec 10, 2014

When I added support for the 'in' operator a couple months ago, I think I got the precedence slightly wrong.

JS has no 'in' operator, so there's nothing to base nunjucks' behavior off there, but Python (https://docs.python.org/2/reference/expressions.html#operator-precedence) and Jinja both bind 'in' more tightly than boolean operators, so that this {% if msg.status in ['pending', 'confirmed'] and msg.body %} does not evaluate as {% if msg.status in (['pending', 'confirmed'] and msg.body) %} (which is what nunjucks master currently does).

I think the Python/Jinja behavior is more useful and logical than nunjucks' current behavior: in is a comparison operator, like == et al, and should bind with similar precedence. You wouldn't expect {% if msg.status == 'pending' and msg.body %} to evaluate as {% if msg.status == ('pending' and msg.body) %}.

This pull request fixes the precedence (and while I was at it, also adds a test to confirm that and binds more tightly than or, which is correct and matches both Python and JS).

@carljm
Copy link
Contributor Author

carljm commented Dec 10, 2014

I guess I can merge this myself, but wouldn't mind a review from somebody first.

@jlongster
Copy link
Contributor

This looks great. Sorry I lapsed on looking at these PRs. Part of the problem was that I started following too many repos and was missing notifications, and I need to setup email filters from github to track them there.

@jlongster
Copy link
Contributor

This is a breaking change, but not too bad, should be ok with a minor version bump (instead of just patch).

jlongster added a commit that referenced this pull request Jan 8, 2015
Bind 'in' operator more tightly than boolean operators.
@jlongster jlongster merged commit 5f0c528 into mozilla:master Jan 8, 2015
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.

2 participants