-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Don't mistake a following if for an elif #5296
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -335,38 +335,37 @@ fn handle_in_between_bodies_end_of_line_comment<'a>( | |
return CommentPlacement::Default(comment); | ||
} | ||
|
||
if !locator.contains_line_break(TextRange::new(preceding.end(), comment.slice().start())) { | ||
// Trailing comment of the preceding statement | ||
if locator.contains_line_break(TextRange::new(preceding.end(), comment.slice().start())) { | ||
// The `elif` or except handlers have their own body to which we can attach the trailing comment | ||
// ```python | ||
// while test: | ||
// a # comment | ||
// else: | ||
// if test: | ||
// a | ||
// elif c: # comment | ||
// b | ||
// ``` | ||
if preceding.is_node_with_body() { | ||
// We can't set this as a trailing comment of the function declaration because it | ||
// will then move behind the function block instead of sticking with the pass | ||
if following.is_except_handler() { | ||
return CommentPlacement::trailing(following, comment); | ||
} else if following.is_stmt_if() { | ||
// We have to exclude for following if statements that are not elif by checking the | ||
// indentation | ||
// ```python | ||
// if True: | ||
// def f(): | ||
// pass # a | ||
// else: | ||
// pass | ||
// else: # Comment | ||
// if False: | ||
// pass | ||
// pass | ||
// ``` | ||
CommentPlacement::Default(comment) | ||
} else { | ||
CommentPlacement::trailing(preceding, comment) | ||
let base_if_indent = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just an idea. It may be annoying to do because of all the type narrowing that's necessary and you would need to identify the right parent body but I thought it may be worth mentioning. The way to normally detect whether it is an if True:
pass
elif False:
pass
if True:
pass
else:
if False:
pass
|
||
whitespace::indentation_at_offset(locator, following.range().start()); | ||
let maybe_elif_indent = whitespace::indentation_at_offset( | ||
locator, | ||
comment.enclosing_node().range().start(), | ||
); | ||
if base_if_indent == maybe_elif_indent { | ||
return CommentPlacement::trailing(following, comment); | ||
} | ||
} | ||
} else if following.is_stmt_if() || following.is_except_handler() { | ||
// The `elif` or except handlers have their own body to which we can attach the trailing comment | ||
// ```python | ||
// if test: | ||
// a | ||
// elif c: # comment | ||
// b | ||
// ``` | ||
CommentPlacement::trailing(following, comment) | ||
} else { | ||
// There are no bodies for the "else" branch and other bodies that are represented as a `Vec<Stmt>`. | ||
// This means, there's no good place to attach the comments to. | ||
// Make this a dangling comments and manually format the comment in | ||
|
@@ -381,6 +380,28 @@ fn handle_in_between_bodies_end_of_line_comment<'a>( | |
// print("nooop") | ||
// ``` | ||
CommentPlacement::dangling(comment.enclosing_node(), comment) | ||
} else { | ||
// Trailing comment of the preceding statement | ||
// ```python | ||
// while test: | ||
// a # comment | ||
// else: | ||
// b | ||
// ``` | ||
if preceding.is_node_with_body() { | ||
// We can't set this as a trailing comment of the function declaration because it | ||
// will then move behind the function block instead of sticking with the pass | ||
// ```python | ||
// if True: | ||
// def f(): | ||
// pass # a | ||
// else: | ||
// pass | ||
// ``` | ||
CommentPlacement::Default(comment) | ||
} else { | ||
CommentPlacement::trailing(preceding, comment) | ||
} | ||
} | ||
} else { | ||
CommentPlacement::Default(comment) | ||
|
@@ -991,14 +1012,22 @@ fn handle_dict_unpacking_comment<'a>( | |
.skip_trivia(); | ||
|
||
// we start from the preceding node but we skip its token | ||
if let Some(first) = tokens.next() { | ||
debug_assert!(matches!( | ||
first, | ||
Token { | ||
kind: TokenKind::LBrace | TokenKind::Comma | TokenKind::Colon, | ||
.. | ||
} | ||
)); | ||
for token in tokens.by_ref() { | ||
// Skip closing parentheses that are not part of the node range | ||
if token.kind == TokenKind::RParen { | ||
continue; | ||
} | ||
debug_assert!( | ||
matches!( | ||
token, | ||
Token { | ||
kind: TokenKind::LBrace | TokenKind::Comma | TokenKind::Colon, | ||
.. | ||
} | ||
), | ||
"{token:?}", | ||
); | ||
break; | ||
} | ||
|
||
// if the remaining tokens from the previous node is exactly `**`, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fix seems unrelated to if/elif right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's already merged: https://github.com/astral-sh/ruff/pull/5293/files#diff-eb676c05c3588ddfa4f15b679292a4418e3b3d1bc5e17ce2ad9e150ff128097aR53
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just confirmed, it's also not part of the merge commit