Merge pull request #502 from sigmavirus24/add-W504

Add W504 for line breaks before binary operators

8 files changed, 129 insertions, 81 deletions

diff --git a/CHANGES.txt b/CHANGES.txt
index 0957be8..af06645 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -6,6 +6,8 @@ UNRELEASED
 
 New checks:
 
+* Add W504 warning for checking that a break doesn't happen after a binary
+  operator. This check is ignored by default
 * Add W605 warning for invalid escape sequences in string literals
 
 2.3.1 (2017-01-31)
diff --git a/pycodestyle.py b/pycodestyle.py
index eb9ff63..d8dfcf4 100755
--- a/pycodestyle.py
+++ b/pycodestyle.py
@@ -80,7 +80,7 @@ except ImportError:
 __version__ = '2.3.1'
 
 DEFAULT_EXCLUDE = '.svn,CVS,.bzr,.hg,.git,__pycache__,.tox'
-DEFAULT_IGNORE = 'E121,E123,E126,E226,E24,E704,W503'
+DEFAULT_IGNORE = 'E121,E123,E126,E226,E24,E704,W503,W504'
 try:
     if sys.platform == 'win32':
         USER_CONFIG = os.path.expanduser(r'~\.pycodestyle')
@@ -1135,34 +1135,26 @@ def explicit_line_join(logical_line, tokens):
                 parens -= 1
 
 
-@register_check
-def break_around_binary_operator(logical_line, tokens):
-    r"""
-    Avoid breaks before binary operators.
+def _is_binary_operator(token_type, text):
+    is_op_token = token_type == tokenize.OP
+    is_conjunction = text in ['and', 'or']
+    # NOTE(sigmavirus24): Previously the not_a_symbol check was executed
+    # conditionally. Since it is now *always* executed, text may be None.
+    # In that case we get a TypeError for `text not in str`.
+    not_a_symbol = text and text not in "()[]{},:.;@=%~"
+    # The % character is strictly speaking a binary operator, but the
+    # common usage seems to be to put it next to the format parameters,
+    # after a line break.
+    return ((is_op_token or is_conjunction) and not_a_symbol)
 
-    The preferred place to break around a binary operator is after the
-    operator, not before it.
 
-    W503: (width == 0\n + height == 0)
-    W503: (width == 0\n and height == 0)
+def _break_around_binary_operators(tokens):
+    """Private function to reduce duplication.
 
-    Okay: (width == 0 +\n height == 0)
-    Okay: foo(\n    -x)
-    Okay: foo(x\n    [])
-    Okay: x = '''\n''' + ''
-    Okay: foo(x,\n    -y)
-    Okay: foo(x,  # comment\n    -y)
-    Okay: var = (1 &\n       ~2)
-    Okay: var = (1 /\n       -2)
-    Okay: var = (1 +\n       -1 +\n       -2)
+    This factors out the shared details between
+    :func:`break_before_binary_operator` and
+    :func:`break_after_binary_operator`.
     """
-    def is_binary_operator(token_type, text):
-        # The % character is strictly speaking a binary operator, but the
-        # common usage seems to be to put it next to the format parameters,
-        # after a line break.
-        return ((token_type == tokenize.OP or text in ['and', 'or']) and
-                text not in "()[]{},:.;@=%~")
-
     line_break = False
     unary_context = True
     # Previous non-newline token types and text
@@ -1174,11 +1166,8 @@ def break_around_binary_operator(logical_line, tokens):
         if ('\n' in text or '\r' in text) and token_type != tokenize.STRING:
             line_break = True
         else:
-            if (is_binary_operator(token_type, text) and line_break and
-                    not unary_context and
-                    not is_binary_operator(previous_token_type,
-                                           previous_text)):
-                yield start, "W503 line break before binary operator"
+            yield (token_type, text, previous_token_type, previous_text,
+                   line_break, unary_context, start)
             unary_context = text in '([{,;'
             line_break = False
             previous_token_type = token_type
@@ -1186,6 +1175,70 @@ def break_around_binary_operator(logical_line, tokens):
 
 
 @register_check
+def break_before_binary_operator(logical_line, tokens):
+    r"""
+    Avoid breaks before binary operators.
+
+    The preferred place to break around a binary operator is after the
+    operator, not before it.
+
+    W503: (width == 0\n + height == 0)
+    W503: (width == 0\n and height == 0)
+    W503: var = (1\n       & ~2)
+    W503: var = (1\n       / -2)
+    W503: var = (1\n       + -1\n       + -2)
+
+    Okay: foo(\n    -x)
+    Okay: foo(x\n    [])
+    Okay: x = '''\n''' + ''
+    Okay: foo(x,\n    -y)
+    Okay: foo(x,  # comment\n    -y)
+    """
+    for context in _break_around_binary_operators(tokens):
+        (token_type, text, previous_token_type, previous_text,
+         line_break, unary_context, start) = context
+        if (_is_binary_operator(token_type, text) and line_break and
+                not unary_context and
+                not _is_binary_operator(previous_token_type,
+                                        previous_text)):
+            yield start, "W503 line break before binary operator"
+
+
+@register_check
+def break_after_binary_operator(logical_line, tokens):
+    r"""
+    Avoid breaks after binary operators.
+
+    The preferred place to break around a binary operator is before the
+    operator, not after it.
+
+    W504: (width == 0 +\n height == 0)
+    W504: (width == 0 and\n height == 0)
+    W504: var = (1 &\n       ~2)
+
+    Okay: foo(\n    -x)
+    Okay: foo(x\n    [])
+    Okay: x = '''\n''' + ''
+    Okay: x = '' + '''\n'''
+    Okay: foo(x,\n    -y)
+    Okay: foo(x,  # comment\n    -y)
+
+    The following should be W504 but unary_context is tricky with these
+    Okay: var = (1 /\n       -2)
+    Okay: var = (1 +\n       -1 +\n       -2)
+    """
+    for context in _break_around_binary_operators(tokens):
+        (token_type, text, previous_token_type, previous_text,
+         line_break, unary_context, start) = context
+        if (_is_binary_operator(previous_token_type, previous_text) and
+                line_break and
+                not unary_context and
+                not _is_binary_operator(token_type, text)):
+            error_pos = (start[0] - 1, start[1])
+            yield error_pos, "W504 line break after binary operator"
+
+
+@register_check
 def comparison_to_singleton(logical_line, noqa):
     r"""Comparison to singletons should use "is" or "is not".
 
diff --git a/setup.cfg b/setup.cfg
index 91ea674..2cd4b2e 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -6,5 +6,5 @@ license_file = LICENSE
 
 [pycodestyle]
 select =
-ignore = E226,E24
+ignore = E226,E24,W504
 max_line_length = 79
diff --git a/testsuite/E12.py b/testsuite/E12.py
index a995c95..acdd81f 100644
--- a/testsuite/E12.py
+++ b/testsuite/E12.py
@@ -20,9 +20,9 @@ print "E124", ("visual",
 #: E124
 a = (123,
 )
-#: E129
-if (row < 0 or self.moduleCount <= row or
-    col < 0 or self.moduleCount <= col):
+#: E129 W503
+if (row < 0 or self.moduleCount <= row
+    or col < 0 or self.moduleCount <= col):
     raise Exception("%s,%s - %s" % (row, col, self.moduleCount))
 #: E126
 print "E126", (
@@ -195,9 +195,9 @@ def qualify_by_address(
     self, cr, uid, ids, context=None,
     params_to_check=frozenset(QUALIF_BY_ADDRESS_PARAM)):
     """ This gets called by the web server """
-#: E129
-if (a == 2 or
-    b == "abc def ghi"
+#: E129 W503
+if (a == 2
+    or b == "abc def ghi"
     "jkl mno"):
     return True
 #:
@@ -225,22 +225,21 @@ rv.update(dict.fromkeys((
 eat_a_dict_a_day({
         "foo": "bar",
 })
-#: E126
+#: E126 W503
 if (
     x == (
             3
-    ) or
-        y == 4):
+    )
+        or y == 4):
     pass
-#: E126
+#: E126 W503 W503
 if (
     x == (
         3
-    ) or
-    x == (
-            3
-    ) or
-        y == 4):
+    )
+    or x == (
+            3)
+        or y == 4):
     pass
 #: E131
 troublesome_hash = {
diff --git a/testsuite/E12not.py b/testsuite/E12not.py
index 6528107..ebaa078 100644
--- a/testsuite/E12not.py
+++ b/testsuite/E12not.py
@@ -1,8 +1,7 @@
 if (
         x == (
             3
-        ) or
-        y == 4):
+        ) or y == 4):
     pass
 
 y = x == 2 \
@@ -17,15 +16,14 @@ if x == 2 \
         or y > 1 \
         or x == 3:
     pass
-
-
-if (foo == bar and
-        baz == frop):
+#: W503
+if (foo == bar
+        and baz == frop):
     pass
-
+#: W503
 if (
-    foo == bar and
-    baz == frop
+    foo == bar
+    and baz == frop
 ):
     pass
 
@@ -109,7 +107,7 @@ sat = 'AAA'    \
       'BBB'    \
       'iii'    \
       'CCC'
-
+#: W504 W504
 abricot = (3 +
            4 +
            5 + 6)
@@ -138,8 +136,7 @@ def long_function_name(
         var_one, var_two, var_three,
         var_four):
     print(var_one)
-
-
+#: W504
 if ((row < 0 or self.moduleCount <= row or
      col < 0 or self.moduleCount <= col)):
     raise Exception("%s,%s - %s" % (row, col, self.moduleCount))
@@ -184,23 +181,23 @@ if bar:
         "to match that of the opening "
         "bracket's line"
     )
-#
+#: W504
 # you want vertical alignment, so use a parens
 if ((foo.bar("baz") and
      foo.bar("frop")
      )):
     print "yes"
-
+#: W504
 # also ok, but starting to look like LISP
 if ((foo.bar("baz") and
      foo.bar("frop"))):
     print "yes"
-
+#: W504
 if (a == 2 or
     b == "abc def ghi"
          "jkl mno"):
     return True
-
+#: W504
 if (a == 2 or
     b == """abc def ghi
 jkl mno"""):
@@ -224,22 +221,19 @@ print 'l.{line}\t{pos}\t{name}\t{text}'.format(
 print('%-7d %s per second (%d total)' % (
       options.counters[key] / elapsed, key,
       options.counters[key]))
-
-
+#: W504
 if os.path.exists(os.path.join(path, PEP8_BIN)):
     cmd = ([os.path.join(path, PEP8_BIN)] +
            self._pep8_options(targetfile))
-
-
+#: W504
 fixed = (re.sub(r'\t+', ' ', target[c::-1], 1)[::-1] +
          target[c + 1:])
-
+#: W504
 fixed = (
     re.sub(r'\t+', ' ', target[c::-1], 1)[::-1] +
     target[c + 1:]
 )
-
-
+#: W504
 if foo is None and bar is "frop" and \
         blah == 'yeah':
     blah = 'yeahnah'
diff --git a/testsuite/W19.py b/testsuite/W19.py
index afdfb76..ed69e2b 100644
--- a/testsuite/W19.py
+++ b/testsuite/W19.py
@@ -7,7 +7,7 @@ if False:
 #: W191
 y = x == 2 \
 	or x == 3
-#: E101 W191
+#: E101 W191 W504
 if (
         x == (
             3
@@ -26,11 +26,11 @@ if x == 2 \
 	pass
 #:
 
-#: E101 W191
+#: E101 W191 W504
 if (foo == bar and
         baz == frop):
 	pass
-#: E101 W191
+#: E101 W191 W504
 if (
     foo == bar and
     baz == frop
@@ -52,7 +52,7 @@ def long_function_name(
         var_one, var_two, var_three,
         var_four):
 	print(var_one)
-#: E101 W191
+#: E101 W191 W504
 if ((row < 0 or self.moduleCount <= row or
      col < 0 or self.moduleCount <= col)):
 	raise Exception("%s,%s - %s" % (row, col, self.moduleCount))
@@ -65,23 +65,23 @@ if bar:
 	    "bracket's line"
 	)
 #
-#: E101 W191
+#: E101 W191 W504
 # you want vertical alignment, so use a parens
 if ((foo.bar("baz") and
      foo.bar("frop")
      )):
 	print "yes"
-#: E101 W191
+#: E101 W191 W504
 # also ok, but starting to look like LISP
 if ((foo.bar("baz") and
      foo.bar("frop"))):
 	print "yes"
-#: E101 W191
+#: E101 W191 W504
 if (a == 2 or
     b == "abc def ghi"
          "jkl mno"):
 	return True
-#: E101 W191
+#: E101 W191 W504
 if (a == 2 or
     b == """abc def ghi
 jkl mno"""):
@@ -93,7 +93,7 @@ if length > options.max_line_length:
 
 
 #
-#: E101 W191 W191
+#: E101 W191 W191 W504
 if os.path.exists(os.path.join(path, PEP8_BIN)):
 	cmd = ([os.path.join(path, PEP8_BIN)] +
 	       self._pep8_options(targetfile))
diff --git a/testsuite/test_all.py b/testsuite/test_all.py
index 08f9ea9..f571a7b 100644
--- a/testsuite/test_all.py
+++ b/testsuite/test_all.py
@@ -43,7 +43,7 @@ class PycodestyleTestCase(unittest.TestCase):
                  os.path.join(ROOT_DIR, 'setup.py')]
         report = self._style.init_report(pycodestyle.StandardReport)
         report = self._style.check_files(files)
-        self.assertFalse(report.total_errors,
+        self.assertEqual(list(report.messages.keys()), ['W504'],
                          msg='Failures: %s' % report.messages)
 
 
diff --git a/testsuite/test_api.py b/testsuite/test_api.py
index 4d8b7b2..6eb9f04 100644
--- a/testsuite/test_api.py
+++ b/testsuite/test_api.py
@@ -166,7 +166,7 @@ class APITestCase(unittest.TestCase):
         self.assertEqual(pep8style.options.filename, ['*.py'])
         self.assertEqual(pep8style.options.format, 'default')
         self.assertEqual(pep8style.options.select, ())
-        self.assertEqual(pep8style.options.ignore, ('E226', 'E24'))
+        self.assertEqual(pep8style.options.ignore, ('E226', 'E24', 'W504'))
         self.assertEqual(pep8style.options.max_line_length, 79)
 
     def test_styleguide_ignore_code(self):
@@ -182,7 +182,7 @@ class APITestCase(unittest.TestCase):
         self.assertEqual(options.select, ())
         self.assertEqual(
             options.ignore,
-            ('E121', 'E123', 'E126', 'E226', 'E24', 'E704', 'W503')
+            ('E121', 'E123', 'E126', 'E226', 'E24', 'E704', 'W503', 'W504')
         )
 
         options = parse_argv('--doctest').options