Repository: google/yapf
Pull request #713 (2019-04-17 09:45:47)

Add knob SPLIT_ALL_TOP_LEVEL_COMMA_SEPARATED_VALUES
Add knob SPLIT_ALL_TOP_LEVEL_COMMA_SEPARATED_VALUES

The existing knob SPLIT_ALL_COMMA_SEPARATED_VALUES is overly aggresive in that, if an argument list (or, more generally, a container) needs splitting, then all subexpressions will be split as well. For instance, in
```
abcdef(aReallyLongThing, b=(c,d))
```
if the line needs splitting because of `aReallyLongThing`, then it will produce
```
abcdef(
    aReallyLongThing,
    b=(c,
       d))
```
I've seen terrible things like
```
   argument: [Int,
              Int]
```
in function definitions with many arguments (even if the second `Int` fits in the line).

The new knob checks if a container (argument list, list literal, etc) fits in a line and avoids breaking if so, producing
```
abcdef(
    aReallyLongThing,
    b=(c, d))
```
which makes more sense (to me at least).

See here for a codebase moved from using split_all_comma_separated_values to split_all_top_level_comma_separated_values in a non-trivial codebase:
https://github.com/prodo-ai/plz/pull/249/files
This file is particularly illustrative.
https://github.com/prodo-ai/plz/pull/249/files#diff-fad3447da9b11c4372ab8fb7754675b2
You can see how it saves a lot of splits here:
https://github.com/prodo-ai/plz/pull/249/files#diff-1174ab209a5ec95fcc62ba032a05596eL124


Patch:
diff --git a/CHANGELOG b/CHANGELOG
index 1821eab..9ca199c 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -3,6 +3,11 @@
 # This project adheres to [Semantic Versioning](http://semver.org/).
 
 ## [0.27.1] UNRELEASED
+### Added
+- New knob `SPLIT_ALL_TOP_LEVEL_COMMA_SEPARATED_VALUES` is a variation on
+  `SPLIT_ALL_COMMA_SEPARATED_VALUES` in which, if a subexpression with a comma
+  fits in its starting line, then the subexpression is not split (thus avoiding
+  unnecessary splits).
 ### Changed
 - Set `INDENT_DICTIONARY_VALUE` for Google style.
 ### Fixed
diff --git a/README.rst b/README.rst
index ead6843..10def1d 100644
--- a/README.rst
+++ b/README.rst
@@ -564,6 +564,27 @@ Knobs
     If a comma separated list (dict, list, tuple, or function def) is on a
     line that is too long, split such that all elements are on a single line.
 
+``SPLIT_ALL_TOP_LEVEL_COMMA_SEPARATED_VALUES``
+    Variation on ``SPLIT_ALL_COMMA_SEPARATED_VALUES`` in which, if a
+    subexpression with a comma fits in its starting line, then the
+    subexpression is not split. This avoids splits like the one for
+    ``b`` in this code:
+
+    .. code-block:: python
+
+    abcdef(
+        aReallyLongThing: int,
+        b: [Int,
+            Int])
+   
+    With the new knob this is split as:
+
+    .. code-block:: python
+
+    abcdef(
+        aReallyLongThing: int,
+        b: [Int, Int])
+
 ``SPLIT_BEFORE_BITWISE_OPERATOR``
     Set to True to prefer splitting before '&', '|' or '^' rather than after.
 
diff --git a/yapf/yapflib/format_decision_state.py b/yapf/yapflib/format_decision_state.py
index a322d38..24ebdff 100644
--- a/yapf/yapflib/format_decision_state.py
+++ b/yapf/yapflib/format_decision_state.py
@@ -176,6 +176,19 @@ class FormatDecisionState(object):
     if style.Get('SPLIT_ALL_COMMA_SEPARATED_VALUES') and previous.value == ',':
       return True
 
+    if (style.Get('SPLIT_ALL_TOP_LEVEL_COMMA_SEPARATED_VALUES') and
+        previous.value == ','):
+      # Avoid breaking in a container that fits in the current line if possible
+      opening = _GetOpeningBracket(current)
+
+      # Can't find opening bracket, behave the same way as
+      # SPLIT_ALL_COMMA_SEPARATED_VALUES
+      if not opening:
+        return True
+
+      # If the container doesn't fit in the current line, must split
+      return not self._ContainerFitsOnStartLine(opening)
+
     if (self.stack[-1].split_before_closing_bracket and
         current.value in '}]' and style.Get('SPLIT_BEFORE_CLOSING_BRACKET')):
       # Split before the closing bracket if we can.
@@ -370,10 +383,7 @@ class FormatDecisionState(object):
 
         opening = _GetOpeningBracket(current)
         if opening:
-          arglist_length = (
-              opening.matching_bracket.total_length - opening.total_length +
-              self.stack[-1].indent)
-          return arglist_length > self.column_limit
+          return not self._ContainerFitsOnStartLine(opening)
 
     if (current.value not in '{)' and previous.value == '(' and
         self._ArgumentListHasDictionaryEntry(current)):
@@ -935,6 +945,18 @@ class FormatDecisionState(object):
         token = token.next_token
     return False
 
+  def _ContainerFitsOnStartLine(self, opening):
+    """Check if the container can fit on its starting line.
+
+    Arguments:
+      opening: (FormatToken) The unwrapped line we're currently processing.
+      
+    Returns:
+      True if the container fits on the start line.
+    """
+    return (opening.matching_bracket.total_length - opening.total_length +
+            self.stack[-1].indent) <= self.column_limit
+
 
 _COMPOUND_STMTS = frozenset(
     {'for', 'while', 'if', 'elif', 'with', 'except', 'def', 'class'})
diff --git a/yapf/yapflib/style.py b/yapf/yapflib/style.py
index 7954c34..48b3984 100644
--- a/yapf/yapflib/style.py
+++ b/yapf/yapflib/style.py
@@ -239,6 +239,9 @@ _STYLE_HELP = dict(
       comma."""),
     SPLIT_ALL_COMMA_SEPARATED_VALUES=textwrap.dedent("""\
       Split before arguments"""),
+    SPLIT_ALL_TOP_LEVEL_COMMA_SEPARATED_VALUES=textwrap.dedent("""\
+      Split before arguments, but do not split all subexpressions recursively
+      (unless needed)."""),
     SPLIT_BEFORE_ARITHMETIC_OPERATOR=textwrap.dedent("""\
       Set to True to prefer splitting before '+', '-', '*', '/', '//', or '@'
       rather than after."""),
@@ -367,6 +370,7 @@ def CreatePEP8Style():
       SPACES_BEFORE_COMMENT=2,
       SPLIT_ARGUMENTS_WHEN_COMMA_TERMINATED=False,
       SPLIT_ALL_COMMA_SEPARATED_VALUES=False,
+      SPLIT_ALL_TOP_LEVEL_COMMA_SEPARATED_VALUES=False,
       SPLIT_BEFORE_ARITHMETIC_OPERATOR=False,
       SPLIT_BEFORE_BITWISE_OPERATOR=True,
       SPLIT_BEFORE_CLOSING_BRACKET=True,
@@ -543,6 +547,7 @@ _STYLE_OPTION_VALUE_CONVERTER = dict(
     SPACES_BEFORE_COMMENT=_IntOrIntListConverter,
     SPLIT_ARGUMENTS_WHEN_COMMA_TERMINATED=_BoolConverter,
     SPLIT_ALL_COMMA_SEPARATED_VALUES=_BoolConverter,
+    SPLIT_ALL_TOP_LEVEL_COMMA_SEPARATED_VALUES=_BoolConverter,
     SPLIT_BEFORE_ARITHMETIC_OPERATOR=_BoolConverter,
     SPLIT_BEFORE_BITWISE_OPERATOR=_BoolConverter,
     SPLIT_BEFORE_CLOSING_BRACKET=_BoolConverter,


Test patch:
diff --git a/yapftests/reformatter_basic_test.py b/yapftests/reformatter_basic_test.py
index 60d7cb6..be3fbbc 100644
--- a/yapftests/reformatter_basic_test.py
+++ b/yapftests/reformatter_basic_test.py
@@ -43,6 +43,76 @@ class BasicReformatterTest(yapf_test_helper.YAPFTest):
               "whatever": 120
           }
           """)
+    uwlines = yapf_test_helper.ParseAndUnwrap(unformatted_code)
+    self.assertCodeEqual(expected_formatted_code, reformatter.Reformat(uwlines))
+    unformatted_code = textwrap.dedent("""\
+          def foo(long_arg, really_long_arg, really_really_long_arg, cant_keep_all_these_args):
+                pass
+          """)
+    expected_formatted_code = textwrap.dedent("""\
+          def foo(long_arg,
+                  really_long_arg,
+                  really_really_long_arg,
+                  cant_keep_all_these_args):
+            pass
+          """)
+    uwlines = yapf_test_helper.ParseAndUnwrap(unformatted_code)
+    self.assertCodeEqual(expected_formatted_code, reformatter.Reformat(uwlines))
+    unformatted_code = textwrap.dedent("""\
+          foo_tuple = [long_arg, really_long_arg, really_really_long_arg, cant_keep_all_these_args]
+          """)
+    expected_formatted_code = textwrap.dedent("""\
+          foo_tuple = [
+              long_arg,
+              really_long_arg,
+              really_really_long_arg,
+              cant_keep_all_these_args
+          ]
+          """)
+    uwlines = yapf_test_helper.ParseAndUnwrap(unformatted_code)
+    self.assertCodeEqual(expected_formatted_code, reformatter.Reformat(uwlines))
+    unformatted_code = textwrap.dedent("""\
+          foo_tuple = [short, arg]
+          """)
+    expected_formatted_code = textwrap.dedent("""\
+          foo_tuple = [short, arg]
+          """)
+    uwlines = yapf_test_helper.ParseAndUnwrap(unformatted_code)
+    self.assertCodeEqual(expected_formatted_code, reformatter.Reformat(uwlines))
+    # There is a test for split_all_top_level_comma_separated_values, with
+    # different expected value
+    unformatted_code = textwrap.dedent("""\
+          someLongFunction(this_is_a_very_long_parameter,
+              abc=(a, this_will_just_fit_xxxxxxx))
+          """)
+    expected_formatted_code = textwrap.dedent("""\
+          someLongFunction(
+              this_is_a_very_long_parameter,
+              abc=(a,
+                   this_will_just_fit_xxxxxxx))
+          """)
+    uwlines = yapf_test_helper.ParseAndUnwrap(unformatted_code)
+    self.assertCodeEqual(expected_formatted_code, reformatter.Reformat(uwlines))
+
+  def testSplittingTopLevelAllArgs(self):
+    style.SetGlobalStyle(
+        style.CreateStyleFromConfig(
+            '{split_all_top_level_comma_separated_values: true, column_limit: 40}'
+        ))
+    # Works the same way as split_all_comma_separated_values
+    unformatted_code = textwrap.dedent("""\
+          responseDict = {"timestamp": timestamp, "someValue":   value, "whatever": 120}
+          """)
+    expected_formatted_code = textwrap.dedent("""\
+          responseDict = {
+              "timestamp": timestamp,
+              "someValue": value,
+              "whatever": 120
+          }
+          """)
+    uwlines = yapf_test_helper.ParseAndUnwrap(unformatted_code)
+    self.assertCodeEqual(expected_formatted_code, reformatter.Reformat(uwlines))
+    # Works the same way as split_all_comma_separated_values
     unformatted_code = textwrap.dedent("""\
           def foo(long_arg, really_long_arg, really_really_long_arg, cant_keep_all_these_args):
                 pass
@@ -56,6 +126,7 @@ class BasicReformatterTest(yapf_test_helper.YAPFTest):
           """)
     uwlines = yapf_test_helper.ParseAndUnwrap(unformatted_code)
     self.assertCodeEqual(expected_formatted_code, reformatter.Reformat(uwlines))
+    # Works the same way as split_all_comma_separated_values
     unformatted_code = textwrap.dedent("""\
           foo_tuple = [long_arg, really_long_arg, really_really_long_arg, cant_keep_all_these_args]
           """)
@@ -69,6 +140,7 @@ class BasicReformatterTest(yapf_test_helper.YAPFTest):
           """)
     uwlines = yapf_test_helper.ParseAndUnwrap(unformatted_code)
     self.assertCodeEqual(expected_formatted_code, reformatter.Reformat(uwlines))
+    # Works the same way as split_all_comma_separated_values
     unformatted_code = textwrap.dedent("""\
           foo_tuple = [short, arg]
           """)
@@ -77,6 +149,46 @@ class BasicReformatterTest(yapf_test_helper.YAPFTest):
           """)
     uwlines = yapf_test_helper.ParseAndUnwrap(unformatted_code)
     self.assertCodeEqual(expected_formatted_code, reformatter.Reformat(uwlines))
+    # There is a test for split_all_comma_separated_values, with different
+    # expected value
+    unformatted_code = textwrap.dedent("""\
+          someLongFunction(this_is_a_very_long_parameter,
+              abc=(a, this_will_just_fit_xxxxxxx))
+          """)
+    expected_formatted_code = textwrap.dedent("""\
+          someLongFunction(
+              this_is_a_very_long_parameter,
+              abc=(a, this_will_just_fit_xxxxxxx))
+          """)
+    uwlines = yapf_test_helper.ParseAndUnwrap(unformatted_code)
+    actual_formatted_code = reformatter.Reformat(uwlines)
+    self.assertEqual(40, len(actual_formatted_code.splitlines()[-1]))
+    self.assertCodeEqual(expected_formatted_code, actual_formatted_code)
+
+    unformatted_code = textwrap.dedent("""\
+          someLongFunction(this_is_a_very_long_parameter,
+              abc=(a, this_will_not_fit_xxxxxxxxx))
+          """)
+    expected_formatted_code = textwrap.dedent("""\
+          someLongFunction(
+              this_is_a_very_long_parameter,
+              abc=(a,
+                   this_will_not_fit_xxxxxxxxx))
+          """)
+    uwlines = yapf_test_helper.ParseAndUnwrap(unformatted_code)
+    self.assertCodeEqual(expected_formatted_code, reformatter.Reformat(uwlines))
+
+    # Exercise the case where there's no opening bracket (for a, b)
+    unformatted_code = textwrap.dedent("""\
+          a, b = f(
+              a_very_long_parameter, yet_another_one, and_another)
+          """)
+    expected_formatted_code = textwrap.dedent("""\
+          a, b = f(
+              a_very_long_parameter, yet_another_one, and_another)
+          """)
+    uwlines = yapf_test_helper.ParseAndUnwrap(unformatted_code)
+    self.assertCodeEqual(expected_formatted_code, reformatter.Reformat(uwlines))
 
   def testSimpleFunctionsWithTrailingComments(self):
     unformatted_code = textwrap.dedent("""\