Skip to content
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

Add new DISABLE_SPLIT_LIST_WITH_COMMENT flag. #1177

Merged
merged 4 commits into from
Nov 8, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This confuses me. You say that with this flag:

we will split lists that contain comments

But isn't that exactly what this flag is supposed to prevent?

Copy link
Contributor Author

@jlebar jlebar Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's right?

You say that with this flag:

we will split lists that contain comments

That's not what I intended to convey. In the context above, "this flag" refers to DISABLE_ENDING_COMMA_HEURISTIC.

To be more clear, here's what changes.

  • Before:

    • Default: split lists with a trailing comma or with comments.
    • DISABLE_ENDING_COMMA_HEURISTIC=true, do not split lists with a trailing comma or with comments.
  • After:

    • Default: Same as before.
    • DISABLE_ENDING_COMMA_HEURISTIC=true and DISABLE_SPLIT_LIST_WITH_COMMENT=false, do not split lists with trailing comma, but do split lists with comments. This is different than before.
    • DISABLE_ENDING_COMMA_HEURISTIC=false and DISABLE_SPLIT_LIST_WITH_COMMENT=true, do split lists with a trailing comma, but do not split lists with comments. You used to get this behavior just by setting DISABLE_ENDING_COMMA_HEURISTIC=true, but now you need to set two flags.
    • Both flags true: do not split lists with a trailing comma, and do not split lists with comments.

(I hope I got that right above.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! Okay.

Could you add the interaction with DISABLE_ENDING_COMMA_HEURISTIC in the README.md and style.py descriptions? I want to make sure people know about any any unforeseen side-effects. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, please have a look. And thanks again!

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.
Expand Down
27 changes: 27 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
21 changes: 16 additions & 5 deletions yapf/pytree/pytree_unwrapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Expand Down
6 changes: 6 additions & 0 deletions yapf/yapflib/style.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
"""),
Expand Down Expand Up @@ -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='',
Expand Down Expand Up @@ -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,
Expand Down
23 changes: 23 additions & 0 deletions yapftests/reformatter_basic_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down