mirror of
https://github.com/astral-sh/ruff.git
synced 2025-11-19 20:24:27 +00:00
Adjust own-line comment placement between branches (#21185)
This PR attempts to improve the placement of own-line comments between
branches in the setting where the comment is more indented than the
preceding node.
There are two main changes.
### First change: Preceding node has leading content
If the preceding node has leading content, we now regard the comment as
automatically _less_ indented than the preceding node, and format
accordingly.
For example,
```python
if True: preceding_node
# leading on `else`, not trailing on `preceding_node`
else: ...
```
This is more compatible with `black`, although there is a (presumably
very uncommon) edge case:
```python
if True:
this;that
# leading on `else`, but trailing in `black`
else: ...
```
I'm sort of okay with this - presumably if one wanted a comment for
those semi-colon separated statements, one should have put it _above_
them, and one wanted a comment only for `that` then it ought to have
been on the same line?
### Second change: searching for last child in body
While searching for the (recursively) last child in the body of the
preceding _branch_, we implicitly assumed that the preceding node had to
have a body to begin the recursion. But actually, in the base case, the
preceding node _is_ the last child in the body of the preceding branch.
So, for example:
```python
if True:
something
last_child_but_no_body
# leading on else for `main` but trailing in this PR
else: ...
```
### More examples
The table below is an attempt to summarize the changes in behavior. The
rows alternate between an example snippet with `while` and the same
example with `if` - in the former case we do _not_ have an `else` node
and in the latter we do.
Notice that:
1. On `main` our handling of `if` vs. `while` is not consistent, whereas
it is consistent in the present PR
2. We disagree with `black` in all cases except that last example on
`main`, but agree in all cases for the present PR (though see above for
a wonky edge case where we disagree).
<table>
<tr>
<th>Original
</th>
<th><code>main</code> </th>
<th>This
PR </th>
<th><code>black</code> </th>
</tr>
<tr>
<td>
<pre lang="python">
while True:
pass
# comment
else:
pass
</pre>
</td>
<td>
<pre lang="python">
while True:
pass
else:
# comment
pass
</pre>
</td>
<td>
<pre lang="python">
while True:
pass
# comment
else:
pass
</pre>
</td>
<td>
<pre lang="python">
while True:
pass
# comment
else:
pass
</pre>
</td>
</tr>
<tr>
<td>
<pre lang="python">
if True:
pass
# comment
else:
pass
</pre>
</td>
<td>
<pre lang="python">
if True:
pass
# comment
else:
pass
</pre>
</td>
<td>
<pre lang="python">
if True:
pass
# comment
else:
pass
</pre>
</td>
<td>
<pre lang="python">
if True:
pass
# comment
else:
pass
</pre>
</td>
</tr>
<tr>
<td>
<pre lang="python">
while True: pass
# comment
else:
pass
</pre>
</td>
<td>
<pre lang="python">
while True:
pass
# comment
else:
pass
</pre>
</td>
<td>
<pre lang="python">
while True:
pass
# comment
else:
pass
</pre>
</td>
<td>
<pre lang="python">
while True:
pass
# comment
else:
pass
</pre>
</td>
</tr>
<tr>
<td>
<pre lang="python">
if True: pass
# comment
else:
pass
</pre>
</td>
<td>
<pre lang="python">
if True:
pass
# comment
else:
pass
</pre>
</td>
<td>
<pre lang="python">
if True:
pass
# comment
else:
pass
</pre>
</td>
<td>
<pre lang="python">
if True:
pass
# comment
else:
pass
</pre>
</td>
</tr>
<tr>
<td>
<pre lang="python">
while True: pass
# comment
else:
pass
</pre>
</td>
<td>
<pre lang="python">
while True:
pass
else:
# comment
pass
</pre>
</td>
<td>
<pre lang="python">
while True:
pass
# comment
else:
pass
</pre>
</td>
<td>
<pre lang="python">
while True:
pass
# comment
else:
pass
</pre>
</td>
</tr>
<tr>
<td>
<pre lang="python">
if True: pass
# comment
else:
pass
</pre>
</td>
<td>
<pre lang="python">
if True:
pass
# comment
else:
pass
</pre>
</td>
<td>
<pre lang="python">
if True:
pass
# comment
else:
pass
</pre>
</td>
<td>
<pre lang="python">
if True:
pass
# comment
else:
pass
</pre>
</td>
</tr>
</table>
This commit is contained in:
Dylan
GitHub
parent
1a86e13472
commit
04a3ec3689
9 changed files with 359 additions and 10 deletions
|
|
@ -294,3 +294,39 @@ if parent_body:
|
|||
# d
|
||||
# e
|
||||
#f
|
||||
|
||||
# Compare behavior with `while`/`else` comment placement
|
||||
|
||||
if True: pass
|
||||
# 1
|
||||
else:
|
||||
pass
|
||||
|
||||
if True:
|
||||
pass
|
||||
# 2
|
||||
else:
|
||||
pass
|
||||
|
||||
if True: pass
|
||||
# 3
|
||||
else:
|
||||
pass
|
||||
|
||||
if True: pass
|
||||
# 4
|
||||
else:
|
||||
pass
|
||||
|
||||
def foo():
|
||||
if True:
|
||||
pass
|
||||
# 5
|
||||
else:
|
||||
pass
|
||||
|
||||
if True:
|
||||
first;second
|
||||
# 6
|
||||
else:
|
||||
pass
|
||||
|
|
|
|||
|
|
@ -28,3 +28,37 @@ while (
|
|||
and anotherCondition or aThirdCondition # trailing third condition
|
||||
): # comment
|
||||
print("Do something")
|
||||
|
||||
while True: pass
|
||||
# 1
|
||||
else:
|
||||
pass
|
||||
|
||||
while True:
|
||||
pass
|
||||
# 2
|
||||
else:
|
||||
pass
|
||||
|
||||
while True: pass
|
||||
# 3
|
||||
else:
|
||||
pass
|
||||
|
||||
while True: pass
|
||||
# 4
|
||||
else:
|
||||
pass
|
||||
|
||||
def foo():
|
||||
while True:
|
||||
pass
|
||||
# 5
|
||||
else:
|
||||
pass
|
||||
|
||||
while True:
|
||||
first;second
|
||||
# 6
|
||||
else:
|
||||
pass
|
||||
|
|
|
|||
|
|
@ -1042,4 +1042,33 @@ else: # trailing comment
|
|||
|
||||
assert_debug_snapshot!(comments.debug(test_case.source_code));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn while_else_indented_comment_between_branches() {
|
||||
let source = r"while True: pass
|
||||
# comment
|
||||
else:
|
||||
pass
|
||||
";
|
||||
let test_case = CommentsTestCase::from_code(source);
|
||||
|
||||
let comments = test_case.to_comments();
|
||||
|
||||
assert_debug_snapshot!(comments.debug(test_case.source_code));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn while_else_very_indented_comment_between_branches() {
|
||||
let source = r"while True:
|
||||
pass
|
||||
# comment
|
||||
else:
|
||||
pass
|
||||
";
|
||||
let test_case = CommentsTestCase::from_code(source);
|
||||
|
||||
let comments = test_case.to_comments();
|
||||
|
||||
assert_debug_snapshot!(comments.debug(test_case.source_code));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -8,7 +8,7 @@ use ruff_python_trivia::{
|
|||
find_only_token_in_range, first_non_trivia_token, indentation_at_offset,
|
||||
};
|
||||
use ruff_source_file::LineRanges;
|
||||
use ruff_text_size::{Ranged, TextLen, TextRange};
|
||||
use ruff_text_size::{Ranged, TextLen, TextRange, TextSize};
|
||||
use std::cmp::Ordering;
|
||||
|
||||
use crate::comments::visitor::{CommentPlacement, DecoratedComment};
|
||||
|
|
@ -602,9 +602,35 @@ fn handle_own_line_comment_between_branches<'a>(
|
|||
// following branch or if it a trailing comment of the previous body's last statement.
|
||||
let comment_indentation = comment_indentation_after(preceding, comment.range(), source);
|
||||
|
||||
let preceding_indentation = indentation(source, &preceding)
|
||||
.unwrap_or_default()
|
||||
.text_len();
|
||||
let preceding_indentation = indentation(source, &preceding).map_or_else(
|
||||
// If `indentation` returns `None`, then there is leading
|
||||
// content before the preceding node. In this case, we
|
||||
// always treat the comment as being less-indented than the
|
||||
// preceding. For example:
|
||||
//
|
||||
// ```python
|
||||
// if True: pass
|
||||
// # leading on `else`
|
||||
// else:
|
||||
// pass
|
||||
// ```
|
||||
// Note we even do this if the comment is very indented
|
||||
// (which matches `black`'s behavior as of 2025.11.11)
|
||||
//
|
||||
// ```python
|
||||
// if True: pass
|
||||
// # leading on `else`
|
||||
// else:
|
||||
// pass
|
||||
// ```
|
||||
|| {
|
||||
comment_indentation
|
||||
// This can be any positive number - we just
|
||||
// want to hit the `Less` branch below
|
||||
+ TextSize::new(1)
|
||||
},
|
||||
ruff_text_size::TextLen::text_len,
|
||||
);
|
||||
|
||||
// Compare to the last statement in the body
|
||||
match comment_indentation.cmp(&preceding_indentation) {
|
||||
|
|
@ -678,8 +704,41 @@ fn handle_own_line_comment_after_branch<'a>(
|
|||
preceding: AnyNodeRef<'a>,
|
||||
source: &str,
|
||||
) -> CommentPlacement<'a> {
|
||||
let Some(last_child) = preceding.last_child_in_body() else {
|
||||
return CommentPlacement::Default(comment);
|
||||
// If the preceding node has a body, we want the last child - e.g.
|
||||
//
|
||||
// ```python
|
||||
// if True:
|
||||
// def foo():
|
||||
// something
|
||||
// last_child
|
||||
// # comment
|
||||
// else:
|
||||
// pass
|
||||
// ```
|
||||
//
|
||||
// Otherwise, the preceding node may be the last statement in the body
|
||||
// of the preceding branch, in which case we can take it as our
|
||||
// `last_child` here - e.g.
|
||||
//
|
||||
// ```python
|
||||
// if True:
|
||||
// something
|
||||
// last_child
|
||||
// # comment
|
||||
// else:
|
||||
// pass
|
||||
// ```
|
||||
let last_child = match preceding.last_child_in_body() {
|
||||
Some(last) => last,
|
||||
None if comment.following_node().is_some_and(|following| {
|
||||
following.is_first_statement_in_alternate_body(comment.enclosing_node())
|
||||
}) =>
|
||||
{
|
||||
preceding
|
||||
}
|
||||
_ => {
|
||||
return CommentPlacement::Default(comment);
|
||||
}
|
||||
};
|
||||
|
||||
// We only care about the length because indentations with mixed spaces and tabs are only valid if
|
||||
|
|
|
|||
|
|
@ -0,0 +1,21 @@
|
|||
---
|
||||
source: crates/ruff_python_formatter/src/comments/mod.rs
|
||||
expression: comments.debug(test_case.source_code)
|
||||
---
|
||||
{
|
||||
Node {
|
||||
kind: StmtWhile,
|
||||
range: 0..45,
|
||||
source: `while True: pass⏎`,
|
||||
}: {
|
||||
"leading": [],
|
||||
"dangling": [
|
||||
SourceComment {
|
||||
text: "# comment",
|
||||
position: OwnLine,
|
||||
formatted: false,
|
||||
},
|
||||
],
|
||||
"trailing": [],
|
||||
},
|
||||
}
|
||||
|
|
@ -0,0 +1,21 @@
|
|||
---
|
||||
source: crates/ruff_python_formatter/src/comments/mod.rs
|
||||
expression: comments.debug(test_case.source_code)
|
||||
---
|
||||
{
|
||||
Node {
|
||||
kind: StmtPass,
|
||||
range: 16..20,
|
||||
source: `pass`,
|
||||
}: {
|
||||
"leading": [],
|
||||
"dangling": [],
|
||||
"trailing": [
|
||||
SourceComment {
|
||||
text: "# comment",
|
||||
position: OwnLine,
|
||||
formatted: false,
|
||||
},
|
||||
],
|
||||
},
|
||||
}
|
||||
|
|
@ -1,7 +1,6 @@
|
|||
---
|
||||
source: crates/ruff_python_formatter/tests/fixtures.rs
|
||||
input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/form_feed.py
|
||||
snapshot_kind: text
|
||||
---
|
||||
## Input
|
||||
```python
|
||||
|
|
@ -18,7 +17,7 @@ else:
|
|||
# Regression test for: https://github.com/astral-sh/ruff/issues/7624
|
||||
if symbol is not None:
|
||||
request["market"] = market["id"]
|
||||
# "remaining_volume": "0.0",
|
||||
# "remaining_volume": "0.0",
|
||||
else:
|
||||
pass
|
||||
```
|
||||
|
|
|
|||
|
|
@ -1,7 +1,6 @@
|
|||
---
|
||||
source: crates/ruff_python_formatter/tests/fixtures.rs
|
||||
input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/if.py
|
||||
snapshot_kind: text
|
||||
---
|
||||
## Input
|
||||
```python
|
||||
|
|
@ -301,6 +300,42 @@ if parent_body:
|
|||
# d
|
||||
# e
|
||||
#f
|
||||
|
||||
# Compare behavior with `while`/`else` comment placement
|
||||
|
||||
if True: pass
|
||||
# 1
|
||||
else:
|
||||
pass
|
||||
|
||||
if True:
|
||||
pass
|
||||
# 2
|
||||
else:
|
||||
pass
|
||||
|
||||
if True: pass
|
||||
# 3
|
||||
else:
|
||||
pass
|
||||
|
||||
if True: pass
|
||||
# 4
|
||||
else:
|
||||
pass
|
||||
|
||||
def foo():
|
||||
if True:
|
||||
pass
|
||||
# 5
|
||||
else:
|
||||
pass
|
||||
|
||||
if True:
|
||||
first;second
|
||||
# 6
|
||||
else:
|
||||
pass
|
||||
```
|
||||
|
||||
## Output
|
||||
|
|
@ -607,6 +642,48 @@ if parent_body:
|
|||
# d
|
||||
# e
|
||||
# f
|
||||
|
||||
# Compare behavior with `while`/`else` comment placement
|
||||
|
||||
if True:
|
||||
pass
|
||||
# 1
|
||||
else:
|
||||
pass
|
||||
|
||||
if True:
|
||||
pass
|
||||
# 2
|
||||
else:
|
||||
pass
|
||||
|
||||
if True:
|
||||
pass
|
||||
# 3
|
||||
else:
|
||||
pass
|
||||
|
||||
if True:
|
||||
pass
|
||||
# 4
|
||||
else:
|
||||
pass
|
||||
|
||||
|
||||
def foo():
|
||||
if True:
|
||||
pass
|
||||
# 5
|
||||
else:
|
||||
pass
|
||||
|
||||
|
||||
if True:
|
||||
first
|
||||
second
|
||||
# 6
|
||||
else:
|
||||
pass
|
||||
```
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -1,7 +1,6 @@
|
|||
---
|
||||
source: crates/ruff_python_formatter/tests/fixtures.rs
|
||||
input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/while.py
|
||||
snapshot_kind: text
|
||||
---
|
||||
## Input
|
||||
```python
|
||||
|
|
@ -35,6 +34,40 @@ while (
|
|||
and anotherCondition or aThirdCondition # trailing third condition
|
||||
): # comment
|
||||
print("Do something")
|
||||
|
||||
while True: pass
|
||||
# 1
|
||||
else:
|
||||
pass
|
||||
|
||||
while True:
|
||||
pass
|
||||
# 2
|
||||
else:
|
||||
pass
|
||||
|
||||
while True: pass
|
||||
# 3
|
||||
else:
|
||||
pass
|
||||
|
||||
while True: pass
|
||||
# 4
|
||||
else:
|
||||
pass
|
||||
|
||||
def foo():
|
||||
while True:
|
||||
pass
|
||||
# 5
|
||||
else:
|
||||
pass
|
||||
|
||||
while True:
|
||||
first;second
|
||||
# 6
|
||||
else:
|
||||
pass
|
||||
```
|
||||
|
||||
## Output
|
||||
|
|
@ -70,4 +103,44 @@ while (
|
|||
or aThirdCondition # trailing third condition
|
||||
): # comment
|
||||
print("Do something")
|
||||
|
||||
while True:
|
||||
pass
|
||||
# 1
|
||||
else:
|
||||
pass
|
||||
|
||||
while True:
|
||||
pass
|
||||
# 2
|
||||
else:
|
||||
pass
|
||||
|
||||
while True:
|
||||
pass
|
||||
# 3
|
||||
else:
|
||||
pass
|
||||
|
||||
while True:
|
||||
pass
|
||||
# 4
|
||||
else:
|
||||
pass
|
||||
|
||||
|
||||
def foo():
|
||||
while True:
|
||||
pass
|
||||
# 5
|
||||
else:
|
||||
pass
|
||||
|
||||
|
||||
while True:
|
||||
first
|
||||
second
|
||||
# 6
|
||||
else:
|
||||
pass
|
||||
```
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue