Don't expand groups for trailing end-of-line comments #6464
Conversation
| a # comment | ||
| == b | ||
| ) | ||
| (a == b) # comment |
There was a problem hiding this comment.
(This now matches Black.)
| a: a # a | ||
| for c in e # for # c # in # e | ||
| } | ||
| {a: a for c in e} # a # for # c # in # e |
There was a problem hiding this comment.
This now matches Black.
| for element in collection # yup | ||
| if element is not None # right | ||
| ] | ||
| lcomp = [element for element in collection if element is not None] # yup # yup # right |
There was a problem hiding this comment.
This is much closer to Black but still wrong due to the line suffixes not being included when determining the line length:
lcomp = [
element for element in collection if element is not None # yup # yup # right
]
charliermarsh
force-pushed
the
charlie/break
branch
from
d536fd5 to
0ee0a6e
Compare
PR Check ResultsBenchmarkLinuxWindows |
charliermarsh
force-pushed
the
charlie/break
branch
from
0ee0a6e to
6783d8a
Compare
|
Interesting to see this change in action. Our formatter goal is to have (close) Black parity for already formatted code, but we intentionally allow deviation for unformatted code (using Black and Ruff in the same project isn't a use case we want to support). I don't expect this change to help increase parity for already Black formatted code because Black already have moved all comments to the end of the line. That's why I'm surprised that it affects the similarity index at all (the overall change seems neutral). We'll have to manually investigate the changes before moving forward. I suspect these are accidental changes where we have a different comment placement, and the lines now happen to be short enough to place the comment in the same position as Black. The main question to me is whether the fewest vertical lines by collapsing comments improve readability and express the author's intentions. In my view, this isn't the case because comments are moved away from where I intentionally placed them: Pragma commentsdef test(
a, # type-ignore
b
):
passThis now gets formatted as def test(a, b): # type-ignore
passwhich not only suppresses typing errors for The workaround with adding trailing commas only works in positions where they are supported. For example, the following doesn't work. (
call(has_type_error) # type-ignore
and another_call()
)gets formatted as (call(has_type_error) and another_call()) # type-ignorewhich suppresses typing errors in # This doesn't work, probably because we have collapsed comments
(
call(has_type_error) # fmt: skip # type-ignore
and another_call()
)
(
# fmt: off
call(has_type_error) # type-ignore
# fmt: on
and another_call()
)Ruff won't be able to provide this escape hatch because it doesn't support suppression comments on the expression level (you would need to suppress the whole statement). And suppression comments are a net negative in my view. Comment contextThis is related to pragma comments. Moving comments too far means that the comments loos their context. call(
[], # TODO fill in values
b,
[]
)gets formatted to call([], b, []) # TODO fill in valuesIt is now unclear if I have to fill in the values in the first or last list. Again, I can work around this by adding a trailing comma, but that's a lot of work to make the formatter respect my comment placement. Similar call(
[], # Empty because of X
c,
[], # Empty because of Y
)gets formatted as call([], c, []) # Empty because of X # Empty because of YAgain, we have the context problem, but I also dislike collapsed comments |
|
We're not making this change, for now at least. |
Summary
This is a PR where we need to align on desired behavior. Right now, we expand a group when writing a trailing end-of-line comment, so e.g., given:
{ a: a # a for c in e # for # c }We format this expression as:
{ a: a # a for c in e # for # c }Black, meanwhile, doesn't let trailing end-of-line comments expand, so formats as:
{a: a for c in e} # a # for # cLooking through the snapshot diff, this seems to strictly improve Black compatibility (and our current behavior seems like a significant deviation). Even the changes in our own fixtures now better resemble Black as verified in the playground.
I don't know that I have a super strong opinion on whether to use Black's behavior or our own, but I lead towards following Black's behavior by default.
Test Plan
Weird mix on the similarity scores -- some went down a little, some went up a little.
Before:
zulip: 0.99702django: 0.99784warehouse: 0.99585build: 0.75623transformers: 0.99469cpython: 0.75988typeshed: 0.74853After:
zulip: 0.99696django: 0.99785warehouse: 0.99569build: 0.75623transformers: 0.99481cpython: 0.75987typeshed: 0.74842