stop having end-of-line comments expand parent #5353
Conversation
seems mostly better, still need to think about
`json = {"k": {"k2": {"k3": [1,]}}}` where we differ from black in
trailing commans for all the outer dicts
this seems more in line with black (and seems to help with `StmtWith`
formatting)
only unexpected change in the diff afict is
```
while (
some_condition(unformatted, args) # trailing some condition
and anotherCondition or aThirdCondition # trailing third condition
): # comment
print("Do something")
```
which for some reason (bug elsewhere?) now doesn't get wrapped
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinuxWindows |
There was a problem hiding this comment.
Thanks for looking into this difference.
This deviation has been a deliberate decision of me, but we haven't decided if we want to keep it.
Overall, our goal is not 100% black compatibility. Our main goal is to format already black formatted files without changes, to ease adoption. My understanding is that the current formatting upholdes this property.
The reasons why I decided to deviate are:
-
Using
expandcan prevent comment migration. This is a problem specific to trailing comments because they potentially move from the end of node A to the end of node B after formatting once. This can be problematic because the formatter may decide for a different layout becausehas_dangling_commentsnow suddenly returnsfalse, or the comment moves even further on the second format and now becomes a trailing comment of node C -
In my view, it's a desired property to respect the user's comment positioning when possible (I mean, we support the weirdest slice comment placements). That means, We shouldn't move the comment from the argument
bin the following example if I deliberately choose to document the argumentb(and not the function header).function( a, b=[] # Using a list here is important because of X ):
Moving the comment to the end of the line changes the context of the comment and it now becomes unclear to what here is referencing. Preserving the placement can further prevent that
type-ignoreor other placement sensitive comments (noqa) are moved or increase scope. -
Aggressively collapsing comments can lead to some weird comment formatting (Ruff can run into this too, but it should happen less often)
# Input while ( cond1 # almost always true and cond2 # almost never true ): print("Do something") # Output while cond1 and cond2: # almost always true # almost never true print("Do something")
What's your perspective on the comment placement? @konstin I remember that you were leaning towards moving comments to the end too
|
|
||
| Ok(()) | ||
| }); | ||
| let items = format_with(|f| f.join_comma_separated().nodes(elts.iter()).finish()); |
There was a problem hiding this comment.
Nice! Would you mind splitting this change into its own PR?
|
thanks for the explanation. l also prefer the current output, but was concerned about black compat, however
this is a great distinction i hadn't considered (no changes to black-formatted code vs same output as black on unformatted code). aside: is this how we use the fixtures from black's test suite? (format with black, _then with ruff). should it be? |
This is a good point. No. The fixtures compare Black's and Ruff's output after formatting an unformatted file. But I like what you're proposing to add another test that uses the expected black output and run it through ruff. |
Generally favourable towards moving them to the end.
I'm fine with a certain amount of comment migration, i often like it when black does that. I feel like having one line comments be fixed and end-of-line comments migrate is a good compromise and gives the user convenience (the formatter will collapse my statements) and control (i can use an own line comment and it will stick).
Those are good points, and i wouldn't be sure how to handle them. |
This resonates with me and I see this working well in an editor. However, it may require manual review when formatting a new project. I added a new item to the formatter task to make a decision and linked to this conversation. |
Do you have an example of a comment migration that you like (esp. one where comments got merged)? |
model.train(
learning_rate=0.0001, # 0.001 doesn't converge
batch=128 # about 8GB
)gets merged into model.train(learning_rate=0.0001, batch=128) # 0.001 doesn't converge # about 8GB |
|
interesting. i would prefer the separate rather than the merged output in that example (note that "that you like" in the above) |
this seems more in line with black (and seems to help with
StmtWithformatting)
only unexpected change in the diff afict is
which for some reason (bug elsewhere?) now doesn't get wrapped
to keep dict formatting from getting worse, first add magic trailing comma support to dicts (separate commit) which
seems mostly better, though still need to think about
json = {"k": {"k2": {"k3": [1,]}}}where we differ from black intrailing commans for all the outer dicts