From 930f03de989c5c88a5c68f255dae570b52d87492 Mon Sep 17 00:00:00 2001 From: konstin Date: Fri, 23 Jun 2023 10:07:28 +0200 Subject: [PATCH] Don't mistake a following if for an elif (#5296) In the following code, the comment used to get wrongly associated with the `if False` since it looked like an elif. This fixes it by checking the indentation and adding a regression test ```python if True: pass else: # Comment if False: pass pass ``` Originally found in https://github.com/gradio-app/gradio/blob/1570b94a02d23d051ae137e0063974fd8a48b34e/gradio/external.py#L478 --- .../test/fixtures/ruff/statement/if.py | 9 +++ .../src/comments/placement.rs | 71 ++++++++++++------- ...r__tests__ruff_test__statement__if_py.snap | 18 +++++ 3 files changed, 73 insertions(+), 25 deletions(-) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/if.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/if.py index 91303ef592..e737ebbab1 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/if.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/if.py @@ -62,3 +62,12 @@ else: pass # Don't drop this comment 3 x = 3 + +# Regression test for a following if that could get confused for an elif +# Originally found in https://github.com/gradio-app/gradio/blob/1570b94a02d23d051ae137e0063974fd8a48b34e/gradio/external.py#L478 +if True: + pass +else: # Comment + if False: + pass + pass diff --git a/crates/ruff_python_formatter/src/comments/placement.rs b/crates/ruff_python_formatter/src/comments/placement.rs index e1cded005f..ca0fec261e 100644 --- a/crates/ruff_python_formatter/src/comments/placement.rs +++ b/crates/ruff_python_formatter/src/comments/placement.rs @@ -335,7 +335,52 @@ 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())) { + 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 + // if test: + // a + // elif c: # comment + // b + // ``` + 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: + // pass + // else: # Comment + // if False: + // pass + // pass + // ``` + let base_if_indent = + 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); + } + } + // There are no bodies for the "else" branch and other bodies that are represented as a `Vec`. + // This means, there's no good place to attach the comments to. + // Make this a dangling comments and manually format the comment in + // in the enclosing node's formatting logic. For `try`, it's the formatters responsibility + // to correctly identify the comments for the `finally` and `orelse` block by looking + // at the comment's range. + // + // ```python + // while x == y: + // pass + // else: # trailing + // print("nooop") + // ``` + CommentPlacement::dangling(comment.enclosing_node(), comment) + } else { // Trailing comment of the preceding statement // ```python // while test: @@ -357,30 +402,6 @@ fn handle_in_between_bodies_end_of_line_comment<'a>( } else { CommentPlacement::trailing(preceding, 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`. - // This means, there's no good place to attach the comments to. - // Make this a dangling comments and manually format the comment in - // in the enclosing node's formatting logic. For `try`, it's the formatters responsibility - // to correctly identify the comments for the `finally` and `orelse` block by looking - // at the comment's range. - // - // ```python - // while x == y: - // pass - // else: # trailing - // print("nooop") - // ``` - CommentPlacement::dangling(comment.enclosing_node(), comment) } } else { CommentPlacement::Default(comment) diff --git a/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__ruff_test__statement__if_py.snap b/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__ruff_test__statement__if_py.snap index a7e33556d9..1c41ddd972 100644 --- a/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__ruff_test__statement__if_py.snap +++ b/crates/ruff_python_formatter/src/snapshots/ruff_python_formatter__tests__ruff_test__statement__if_py.snap @@ -68,6 +68,15 @@ else: pass # Don't drop this comment 3 x = 3 + +# Regression test for a following if that could get confused for an elif +# Originally found in https://github.com/gradio-app/gradio/blob/1570b94a02d23d051ae137e0063974fd8a48b34e/gradio/external.py#L478 +if True: + pass +else: # Comment + if False: + pass + pass ``` @@ -137,6 +146,15 @@ else: pass # Don't drop this comment 3 x = 3 + +# Regression test for a following if that could get confused for an elif +# Originally found in https://github.com/gradio-app/gradio/blob/1570b94a02d23d051ae137e0063974fd8a48b34e/gradio/external.py#L478 +if True: + pass +else: # Comment + if False: + pass + pass ```