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

Allow using comparison operator symbols as tests #665

Merged
merged 1 commit into from Jul 6, 2017

Conversation

3 participants
@davidism
Member

davidism commented Jan 20, 2017

I added the Python short names for the operators as well as the symbols. I kept the long names for the three existing tests.

parse_test is adjusted to allow comparison symbols as test names, so that they can be used in direct tests as well as filters.

{{ x is <= 5 }} isn't very useful, but it's allowed for consistency
{{ x is le 5 }}
{{ x|select('<=', 5) }}
{{ x|select('le', 5) }}

Todo:

  • unit tests
  • documentation
  • changelog

closes #664

@davidism davidism self-assigned this Jan 20, 2017

@davidism davidism requested a review from mitsuhiko Jan 20, 2017

Show outdated Hide outdated jinja2/tests.py
test_equalto = test_compare_eq
test_greaterthan = test_compare_gt
test_lessthan = test_compare_lt

This comment has been minimized.

@davidism

davidism Jan 20, 2017

Member

I renamed the functions for consistency with Python's names. These three names already existed, so I kept the aliases, even though I couldn't find any direct use of them with GitHub search.

@davidism

davidism Jan 20, 2017

Member

I renamed the functions for consistency with Python's names. These three names already existed, so I kept the aliases, even though I couldn't find any direct use of them with GitHub search.

This comment has been minimized.

@jeffwidman

jeffwidman Jan 20, 2017

Member

What's the best way to mark as deprecated so they can be removed in the future?

@jeffwidman

jeffwidman Jan 20, 2017

Member

What's the best way to mark as deprecated so they can be removed in the future?

This comment has been minimized.

@davidism

davidism Jan 20, 2017

Member

I could make them functions that raise deprecation warnings, but I'm not that concerned. I'll just add a comment to the source.

@davidism

davidism Jan 20, 2017

Member

I could make them functions that raise deprecation warnings, but I'm not that concerned. I'll just add a comment to the source.

Show outdated Hide outdated jinja2/tests.py
'in': test_in
'==': test_compare_eq,
'eq': test_compare_eq,
'equalto': test_compare_eq,

This comment has been minimized.

@davidism

davidism Jan 20, 2017

Member

Similarly, I kept the long names for the three tests that already existed, in addition to the operator and short names.

@davidism

davidism Jan 20, 2017

Member

Similarly, I kept the long names for the three tests that already existed, in addition to the operator and short names.

@ThiefMaster

This comment has been minimized.

Show comment
Hide comment
@ThiefMaster

ThiefMaster Jan 20, 2017

Member

Is it really a good idea to allow symbol operators in "normal" tests, i.e. for stuff like foo is == 'bar'? That looks completely awful to me to be honest. I'd rather allow an "extended set of tests" for the places where you use them as strings, i.e. select, reject and their ...attr variants.

This would also add the advantage that editors and IDEs won't have to deal with new syntax when highlighting/checking Jinja code - I'm quite quire that PyCharm is not the only IDE that currently shows {% if foo is == 'bar' %} as a syntax error.

Member

ThiefMaster commented Jan 20, 2017

Is it really a good idea to allow symbol operators in "normal" tests, i.e. for stuff like foo is == 'bar'? That looks completely awful to me to be honest. I'd rather allow an "extended set of tests" for the places where you use them as strings, i.e. select, reject and their ...attr variants.

This would also add the advantage that editors and IDEs won't have to deal with new syntax when highlighting/checking Jinja code - I'm quite quire that PyCharm is not the only IDE that currently shows {% if foo is == 'bar' %} as a syntax error.

Show outdated Hide outdated jinja2/tests.py
'greaterthan': test_greaterthan,
'lessthan': test_lessthan,
'in': test_in
'==': test_compare_eq,

This comment has been minimized.

@ThiefMaster

ThiefMaster Jan 20, 2017

Member

I wouldn't use custom functions but operator.eq etc. - they do the same in the end but are faster.

@ThiefMaster

ThiefMaster Jan 20, 2017

Member

I wouldn't use custom functions but operator.eq etc. - they do the same in the end but are faster.

@davidism

This comment has been minimized.

Show comment
Hide comment
@davidism

davidism Jan 20, 2017

Member

@ThiefMaster I thought it was a bit weird too, but I figured people would get confused by the inconsistency between is and select otherwise. The docs already call out equalto as being useless in is.

Since there was no expectation before that symbols could be used, we could just document them as a special case in the select docs, rather than in the general test docs.

Or we could still allow them in both locations, but only document them in select. I can open a PyCharm issue for the syntax, although we both know it could take a while to get in.

Member

davidism commented Jan 20, 2017

@ThiefMaster I thought it was a bit weird too, but I figured people would get confused by the inconsistency between is and select otherwise. The docs already call out equalto as being useless in is.

Since there was no expectation before that symbols could be used, we could just document them as a special case in the select docs, rather than in the general test docs.

Or we could still allow them in both locations, but only document them in select. I can open a PyCharm issue for the syntax, although we both know it could take a while to get in.

Show outdated Hide outdated jinja2/tests.py
test_greaterthan = test_compare_gt
test_lessthan = test_compare_lt
# deprecated aliases, use the operator module instead
test_equalto = operator.eq

This comment has been minimized.

@ThiefMaster

ThiefMaster Jan 20, 2017

Member

Were these functions ever documented as a public API?

@ThiefMaster

ThiefMaster Jan 20, 2017

Member

Were these functions ever documented as a public API?

This comment has been minimized.

@davidism

davidism Jan 20, 2017

Member

They weren't. I was considering just removing them, since it's an obvious failure and fix in the off chance someone is actually importing them.

@davidism

davidism Jan 20, 2017

Member

They weren't. I was considering just removing them, since it's an obvious failure and fix in the off chance someone is actually importing them.

@davidism davidism removed the request for review from mitsuhiko Jan 23, 2017

@davidism

This comment has been minimized.

Show comment
Hide comment
@davidism

davidism Jul 6, 2017

Member

Going to take out the is <= support, since I agree it looks weird. Here's the patch if we decide we need this later.

diff --git a/jinja2/parser.py b/jinja2/parser.py
index 6d1fff6a..8741520a 100644
--- a/jinja2/parser.py
+++ b/jinja2/parser.py
@@ -825,10 +825,19 @@ def parse_test(self, node):
             negated = True
         else:
             negated = False
-        name = self.stream.expect('name').value
-        while self.stream.current.type == 'dot':
-            next(self.stream)
-            name += '.' + self.stream.expect('name').value
+
+        # If the next token is a comparison operator, treat it as a name.
+        # This allows using the operator symbols in the select filter without
+        # breaking unexpectedly when used in a test node.
+        if self.stream.current.type in _compare_operators:
+            name = next(self.stream).value
+        else:
+            name = self.stream.expect('name').value
+
+            while self.stream.current.type == 'dot':
+                next(self.stream)
+                name += '.' + self.stream.expect('name').value
+
         dyn_args = dyn_kwargs = None
         kwargs = []
         if self.stream.current.type == 'lparen':
Member

davidism commented Jul 6, 2017

Going to take out the is <= support, since I agree it looks weird. Here's the patch if we decide we need this later.

diff --git a/jinja2/parser.py b/jinja2/parser.py
index 6d1fff6a..8741520a 100644
--- a/jinja2/parser.py
+++ b/jinja2/parser.py
@@ -825,10 +825,19 @@ def parse_test(self, node):
             negated = True
         else:
             negated = False
-        name = self.stream.expect('name').value
-        while self.stream.current.type == 'dot':
-            next(self.stream)
-            name += '.' + self.stream.expect('name').value
+
+        # If the next token is a comparison operator, treat it as a name.
+        # This allows using the operator symbols in the select filter without
+        # breaking unexpectedly when used in a test node.
+        if self.stream.current.type in _compare_operators:
+            name = next(self.stream).value
+        else:
+            name = self.stream.expect('name').value
+
+            while self.stream.current.type == 'dot':
+                next(self.stream)
+                name += '.' + self.stream.expect('name').value
+
         dyn_args = dyn_kwargs = None
         kwargs = []
         if self.stream.current.type == 'lparen':
allow using comparison operator symbols as tests
add tests and aliases for all comparison operators
adjust docs to prefer short names for compare tests
closes #664

@davidism davidism merged commit ca1abd4 into master Jul 6, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@davidism davidism deleted the operator-tests branch Jul 6, 2017

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