Add new DISABLE_SPLIT_LIST_WITH_COMMENT flag. (#1177)

`DISABLE_SPLIT_LIST_WITH_COMMENT` is a new knob that changes the
behavior of splitting a list when a comment is present inside the list.

Before, we split a list containing a comment just like we split a list
containing a trailing comma: Each element goes on its own line (unless
`DISABLE_ENDING_COMMA_HEURISTIC` is true).

Now, if `DISABLE_SPLIT_LIST_WITH_COMMENT` is true, we do not split every
element of the list onto a new line just because there's a comment somewhere
in the list.

This mirrors the behavior of clang-format, and is useful for e.g. forming
"logical groups" of elements in a list.

Note: Upgrading will result in a behavioral change if you have
`DISABLE_ENDING_COMMA_HEURISTIC` in your config.  Before this version, this
flag caused us not to split lists with a trailing comma *and* lists that
contain comments.  Now, if you set only that flag, we *will* split lists
that contain comments.  Set the new `DISABLE_SPLIT_LIST_WITH_COMMENT` flag to
true to preserve the old behavior.

Author: jlebar

CHANGELOG.md

@@ -2,7 +2,70 @@
 # All notable changes to this project will be documented in this file.
 # This project adheres to [Semantic Versioning](http://semver.org/).
 
-## (0.40.3) UNRELEASED
+## (0.41.0) UNRELEASED
+### Added
+- New `DISABLE_SPLIT_LIST_WITH_COMMENT` flag.
+ `DISABLE_SPLIT_LIST_WITH_COMMENT` is a new knob that changes the
+  behavior of splitting a list when a comment is present inside the list.
+
+  Before, we split a list containing a comment just like we split a list
+  containing a trailing comma: Each element goes on its own line (unless
+  `DISABLE_ENDING_COMMA_HEURISTIC` is true).
+
+  This new flag allows you to control the behavior of a list with a comment
+  *separately* from the behavior when the list contains a trailing comma.
+
+  This mirrors the behavior of clang-format, and is useful for e.g. forming
+  "logical groups" of elements in a list.
+
+  Without this flag:
+
+  ```
+  [
+    a,
+    b,  #
+    c
+  ]
+  ```
+
+  With this flag:
+
+  ```
+  [
+    a, b,  #
+    c
+  ]
+  ```
+
+  Before we had one flag that controlled two behaviors.
+
+    - `DISABLE_ENDING_COMMA_HEURISTIC=false` (default):
+      - Split a list that has a trailing comma.
+      - Split a list that contains a comment.
+    - `DISABLE_ENDING_COMMA_HEURISTIC=true`:
+      - Don't split on trailing comma.
+      - Don't split on comment.
+
+  Now we have two flags.
+
+    - `DISABLE_ENDING_COMMA_HEURISTIC=false` and `DISABLE_SPLIT_LIST_WITH_COMMENT=false` (default):
+      - Split a list that has a trailing comma.
+      - Split a list that contains a comment.
+      Behavior is unchanged from the default before.
+    - `DISABLE_ENDING_COMMA_HEURISTIC=true` and `DISABLE_SPLIT_LIST_WITH_COMMENT=false` :
+      - Don't split on trailing comma.
+      - Do split on comment.  **This is a change in behavior from before.**
+    - `DISABLE_ENDING_COMMA_HEURISTIC=false` and `DISABLE_SPLIT_LIST_WITH_COMMENT=true` :
+      - Split on trailing comma.
+      - Don't split on comment.
+    - `DISABLE_ENDING_COMMA_HEURISTIC=true` and `DISABLE_SPLIT_LIST_WITH_COMMENT=true` :
+      - Don't split on trailing comma.
+      - Don't split on comment.
+      **You used to get this behavior just by setting one flag, but now you have to set both.**
+
+  Note the behavioral change above; if you set
+  `DISABLE_ENDING_COMMA_HEURISTIC=true` and want to keep the old behavior, you
+  now also need to set `DISABLE_SPLIT_LIST_WITH_COMMENT=true`.
 ### Changes
 - Remove dependency on importlib-metadata
 - Remove dependency on tomli when using >= py311

README.md

@@ -512,6 +512,41 @@ optional arguments:
 
 >    Disable the heuristic which places each list element on a separate line if
 >    the list is comma-terminated.
+>
+>    Note: The behavior of this flag changed in v0.40.3.  Before, if this flag
+>    was true, we would split lists that contained a trailing comma or a
+>    comment.  Now, we have a separate flag, `DISABLE_SPLIT_LIST_WITH_COMMENT`,
+>    that controls splitting when a list contains a comment.  To get the old
+>    behavior, set both flags to true.  More information in
+>    [CHANGELOG.md](CHANGELOG.md#new-disable_split_list_with_comment-flag).
+
+#### `DISABLE_DISABLE_SPLIT_LIST_WITH_COMMENT` (new in 0.40.3)
+
+>    Don't put every element on a new line within a list that contains
+>    interstitial comments.
+>
+>    Without this flag (default):
+>
+>    ```
+>    [
+>      a,
+>      b,  #
+>      c
+>    ]
+>    ```
+>
+>    With this flag:
+>
+>    ```
+>    [
+>      a, b,  #
+>      c
+>    ]
+>    ```
+>
+>    This mirrors the behavior of clang-format and is useful for forming
+>    "logical groups" of elements in a list.  It also works in function
+>    declarations.
 
 #### `EACH_DICT_ENTRY_ON_SEPARATE_LINE`
 

yapf/pytree/pytree_unwrapper.py

@@ -407,16 +407,27 @@ def _AdjustSplitPenalty(line):
 
 def _DetermineMustSplitAnnotation(node):
   """Enforce a split in the list if the list ends with a comma."""
-  if style.Get('DISABLE_ENDING_COMMA_HEURISTIC'):
-    return
-  if not _ContainsComments(node):
+
+  def SplitBecauseTrailingComma():
+    if style.Get('DISABLE_ENDING_COMMA_HEURISTIC'):
+      return False
     token = next(node.parent.leaves())
     if token.value == '(':
       if sum(1 for ch in node.children if ch.type == grammar_token.COMMA) < 2:
-        return
+        return False
     if (not isinstance(node.children[-1], pytree.Leaf) or
         node.children[-1].value != ','):
-      return
+      return False
+    return True
+
+  def SplitBecauseListContainsComment():
+    return (not style.Get('DISABLE_SPLIT_LIST_WITH_COMMENT') and
+            _ContainsComments(node))
+
+  if (not SplitBecauseTrailingComma() and
+      not SplitBecauseListContainsComment()):
+    return
+
   num_children = len(node.children)
   index = 0
   _SetMustSplitOnFirstLeaf(node.children[0])

yapf/yapflib/style.py

@@ -179,6 +179,16 @@ def method():
     DISABLE_ENDING_COMMA_HEURISTIC=textwrap.dedent("""\
       Disable the heuristic which places each list element on a separate line
       if the list is comma-terminated.
+
+      Note: The behavior of this flag changed in v0.40.3.  Before, if this flag
+      was true, we would split lists that contained a trailing comma or a
+      comment.  Now, we have a separate flag, `DISABLE_SPLIT_LIT_WITH_COMMENT`,
+      that controls splitting when a list contains a comment.  To get the old
+      behavior, set both flags to true.  More information in CHANGELOG.md.
+    """),
+    DISABLE_SPLIT_LIST_WITH_COMMENT=textwrap.dedent("""
+      Don't put every element on a new line within a list that contains
+      interstitial comments.
     """),
     EACH_DICT_ENTRY_ON_SEPARATE_LINE=textwrap.dedent("""\
       Place each dictionary entry onto its own line.
@@ -483,6 +493,7 @@ def CreatePEP8Style():
       CONTINUATION_INDENT_WIDTH=4,
       DEDENT_CLOSING_BRACKETS=False,
       DISABLE_ENDING_COMMA_HEURISTIC=False,
+      DISABLE_SPLIT_LIST_WITH_COMMENT=False,
       EACH_DICT_ENTRY_ON_SEPARATE_LINE=True,
       FORCE_MULTILINE_DICT=False,
       I18N_COMMENT='',
@@ -671,6 +682,7 @@ def _IntOrIntListConverter(s):
     CONTINUATION_INDENT_WIDTH=int,
     DEDENT_CLOSING_BRACKETS=_BoolConverter,
     DISABLE_ENDING_COMMA_HEURISTIC=_BoolConverter,
+    DISABLE_SPLIT_LIST_WITH_COMMENT=_BoolConverter,
     EACH_DICT_ENTRY_ON_SEPARATE_LINE=_BoolConverter,
     FORCE_MULTILINE_DICT=_BoolConverter,
     I18N_COMMENT=str,

yapftests/reformatter_basic_test.py

@@ -345,6 +345,29 @@ def f(  # Intermediate comment
     llines = yapf_test_helper.ParseAndUnwrap(unformatted_code)
     self.assertCodeEqual(expected_formatted_code, reformatter.Reformat(llines))
 
+  def testParamListWithTrailingComments(self):
+    unformatted_code = textwrap.dedent("""\
+        def f(a,
+              b, #
+              c):
+          pass
+    """)
+    expected_formatted_code = textwrap.dedent("""\
+        def f(a, b,  #
+              c):
+          pass
+    """)
+    try:
+      style.SetGlobalStyle(
+          style.CreateStyleFromConfig(
+              '{based_on_style: yapf,'
+              ' disable_split_list_with_comment: True}'))
+      llines = yapf_test_helper.ParseAndUnwrap(unformatted_code)
+      self.assertCodeEqual(expected_formatted_code,
+                           reformatter.Reformat(llines))
+    finally:
+      style.SetGlobalStyle(style.CreateYapfStyle())
+
   def testBlankLinesBetweenTopLevelImportsAndVariables(self):
     unformatted_code = textwrap.dedent("""\
         import foo as bar