[ruff] Needless else clause (RUF047)
#15051
Conversation
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| RUF047 | 6 | 6 | 0 | 0 | 0 |
MichaReiser
added
rule
There was a problem hiding this comment.
Thanks.
I refactored your rule to remove some duplication and simplified it to only detect single pass or ... statements. PIE790 detects bodies containing both a ... and a pass statement.
I further extended the test cases and there's one case where we fail to detect a useless else because of the comment and one case where we marked the else as useless when we should not. We have to take the indent of the comments into consideration. See
andMaybe we can come up with a simpler heuristic for this specific case?
| /// Checks for `else` clauses that only contains `pass` and `...` statements. | ||
| /// | ||
| /// ## Why is this bad? | ||
| /// Such a clause is unnecessary. |
There was a problem hiding this comment.
| /// Such a clause is unnecessary. | |
| /// Such an else clause does nothing and can be removed. |
| } | ||
|
|
||
| fn fix_title(&self) -> String { | ||
| "Remove clause".to_string() |
There was a problem hiding this comment.
| "Remove clause".to_string() | |
| "Remove the `else` clause".to_string() |
| #[test_case(Rule::NeedlessElse, Path::new("RUF047_if.py"))] | ||
| #[test_case(Rule::NeedlessElse, Path::new("RUF047_for.py"))] | ||
| #[test_case(Rule::NeedlessElse, Path::new("RUF047_while.py"))] | ||
| #[test_case(Rule::NeedlessElse, Path::new("RUF047_try.py"))] |
There was a problem hiding this comment.
The needless-else rule has no mode.is_preview checks. We can add it to the normal rules test, which simplifies promoting them to stable.
| RUF047_if.py:41:1: RUF047 [*] Empty `else` clause | ||
| | | ||
| 39 | if of_course(): | ||
| 40 | this() | ||
| 41 | / else: | ||
| 42 | | ... | ||
| | |_______^ RUF047 | ||
| 43 | # comment | ||
| | | ||
| = help: Remove clause |
There was a problem hiding this comment.
We should not suggest this fix because the comment belongs to the else clause
| if this_second_comment(): | ||
| belongs() # to | ||
| # `else` | ||
| else: | ||
| pass | ||
|
|
There was a problem hiding this comment.
We should detect that this else branch is useless because the comment belongs to the if block
|
@MichaReiser If indentation is to be taken into consideration, should this be reported or not? if test:
_4_spaces()
# 2 spaces
else:
...What about this? if test:
_1_tab() # These two lines would align if tabs are displayed as 1 character wide.
# 1 space
else:
...
# Same question, but for 2, 3, 4 spaces? |
|
We should implement it so that it's consistent with the formatter. Your examples get formatted to if test:
_1_tab() # These two lines would align if tabs are displayed as 1 character wide.
# 1 space
else:
...
# Same question, but for 2, 3, 4 spaces?
if test:
_4_spaces()
# 2 spaces
else:
... |
Summary
Partially addresses #13929.
Test Plan
cargo nextest runandcargo insta test.