From bf301f5ef7777e137b97219842629ca78eb5ef2a Mon Sep 17 00:00:00 2001 From: Justin Lebar Date: Thu, 2 Nov 2023 14:53:49 -0700 Subject: [PATCH 1/3] Add 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). 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. --- CHANGELOG.md | 22 ++++++++++++++++++++++ README.md | 27 +++++++++++++++++++++++++++ yapf/pytree/pytree_unwrapper.py | 21 ++++++++++++++++----- yapf/yapflib/style.py | 6 ++++++ yapftests/reformatter_basic_test.py | 24 ++++++++++++++++++++++++ 5 files changed, 95 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ca880fb0e..8e4e2b4d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,28 @@ ### Changes - Remove dependency on importlib-metadata - Remove dependency on tomli when using >= py311 +### Added +- `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. + ### Fixed - Fix SPLIT_ARGUMENTS_WHEN_COMMA_TERMINATED for one-item named argument lists by taking precedence over SPLIT_BEFORE_NAMED_ASSIGNS. diff --git a/README.md b/README.md index 5ce50b764..a76c2e314 100644 --- a/README.md +++ b/README.md @@ -513,6 +513,33 @@ optional arguments: > Disable the heuristic which places each list element on a separate line if > the list is comma-terminated. +#### `DISABLE_DISABLE_SPLIT_LIST_WITH_COMMENT` + +> 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 is useful for forming "logical groups" of elements in a list. It also +> works in function declarations. + #### `EACH_DICT_ENTRY_ON_SEPARATE_LINE` > Place each dictionary entry onto its own line. diff --git a/yapf/pytree/pytree_unwrapper.py b/yapf/pytree/pytree_unwrapper.py index 4b84cd54c..80e050fbd 100644 --- a/yapf/pytree/pytree_unwrapper.py +++ b/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]) diff --git a/yapf/yapflib/style.py b/yapf/yapflib/style.py index b43437ee0..42030abc4 100644 --- a/yapf/yapflib/style.py +++ b/yapf/yapflib/style.py @@ -180,6 +180,10 @@ def method(): Disable the heuristic which places each list element on a separate line if the list is comma-terminated. """), + 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 +487,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 +676,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, diff --git a/yapftests/reformatter_basic_test.py b/yapftests/reformatter_basic_test.py index d58343e7c..6c7521bfd 100644 --- a/yapftests/reformatter_basic_test.py +++ b/yapftests/reformatter_basic_test.py @@ -345,6 +345,30 @@ 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 + """) + llines = yapf_test_helper.ParseAndUnwrap(unformatted_code) + 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 From 6dc5adb959b371f4fa69b27b799cbade94a7cae2 Mon Sep 17 00:00:00 2001 From: Justin Lebar Date: Thu, 2 Nov 2023 18:10:08 -0700 Subject: [PATCH 2/3] Fix copy-paste mistake in test. --- yapftests/reformatter_basic_test.py | 1 - 1 file changed, 1 deletion(-) diff --git a/yapftests/reformatter_basic_test.py b/yapftests/reformatter_basic_test.py index 6c7521bfd..74b1ba405 100644 --- a/yapftests/reformatter_basic_test.py +++ b/yapftests/reformatter_basic_test.py @@ -357,7 +357,6 @@ def f(a, b, # c): pass """) - llines = yapf_test_helper.ParseAndUnwrap(unformatted_code) try: style.SetGlobalStyle( style.CreateStyleFromConfig( From cc2b6a7466df3ea8a42f981c215e6b6a901624c3 Mon Sep 17 00:00:00 2001 From: Justin Lebar Date: Wed, 8 Nov 2023 11:09:47 -0800 Subject: [PATCH 3/3] Update docs. Write more in CHANGELOG.md, and add pointers to the changelog in README.md and style.py. --- CHANGELOG.md | 61 ++++++++++++++++++++++++++++++++++++------- README.md | 14 +++++++--- yapf/yapflib/style.py | 6 +++++ 3 files changed, 69 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e4e2b4d6..77a6d801f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ - Remove dependency on importlib-metadata - Remove dependency on tomli when using >= py311 ### 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. @@ -14,19 +16,60 @@ 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 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. - 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. + 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`. ### Fixed - Fix SPLIT_ARGUMENTS_WHEN_COMMA_TERMINATED for one-item named argument lists diff --git a/README.md b/README.md index a76c2e314..0e4282f65 100644 --- a/README.md +++ b/README.md @@ -512,8 +512,15 @@ 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` +#### `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. @@ -537,8 +544,9 @@ optional arguments: > ] > ``` > -> This is useful for forming "logical groups" of elements in a list. It also -> works in function declarations. +> 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` diff --git a/yapf/yapflib/style.py b/yapf/yapflib/style.py index 42030abc4..7642c01f4 100644 --- a/yapf/yapflib/style.py +++ b/yapf/yapflib/style.py @@ -179,6 +179,12 @@ 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