Skip to content

Commit

Permalink
Preserve trivia (i.e. comments) in PLR5501 (#13573)
Browse files Browse the repository at this point in the history 
Closes #13545

As described in the issue, we move comments before the inner `if`
statement to before the newly constructed `elif` statement (previously
`else`).
  • Loading branch information
@zanieb
zanieb authored Oct 3, 2024
1 parent fdd0a22 commit cc1f766
Show file tree
Hide file tree
Showing 3 changed files with 225 additions and 23 deletions.
47 changes: 47 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pylint/collapsible_else_if.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,3 +97,50 @@ def not_ok5():
if 2:
pass
else: pass


def not_ok1_with_multiline_comments():
if 1:
pass
else:
# inner comment which happens
# to be longer than one line
if 2:
pass
else:
pass # final pass comment


def not_ok1_with_deep_indented_comments():
if 1:
pass
else:
# inner comment which happens to be overly indented
if 2:
pass
else:
pass # final pass comment


def not_ok1_with_shallow_indented_comments():
if 1:
pass
else:
# inner comment which happens to be under indented
if 2:
pass
else:
pass # final pass comment


def not_ok1_with_mixed_indented_comments():
if 1:
pass
else:
# inner comment which has mixed
# indentation levels
# which is pretty weird
if 2:
pass
else:
pass # final pass comment
31 changes: 25 additions & 6 deletions crates/ruff_linter/src/rules/pylint/rules/collapsible_else_if.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,11 @@ fn convert_to_elif(
let inner_if_line_start = locator.line_start(first.start());
let inner_if_line_end = locator.line_end(first.end());

// Identify the indentation of the loop itself (e.g., the `while` or `for`).
// Capture the trivia between the `else` and the `if`.
let else_line_end = locator.full_line_end(else_clause.start());
let trivia_range = TextRange::new(else_line_end, inner_if_line_start);

// Identify the indentation of the outer clause
let Some(indentation) = indentation(locator, else_clause) else {
return Err(anyhow::anyhow!("`else` is expected to be on its own line"));
};
Expand All @@ -122,15 +126,30 @@ fn convert_to_elif(
stylist,
)?;

// If there's trivia, restore it
let trivia = if trivia_range.is_empty() {
None
} else {
let indented_trivia =
adjust_indentation(trivia_range, indentation, locator, indexer, stylist)?;
Some(Edit::insertion(
indented_trivia,
locator.line_start(else_clause.start()),
))
};

// Strip the indent from the first line of the `if` statement, and add `el` to the start.
let Some(unindented) = indented.strip_prefix(indentation) else {
return Err(anyhow::anyhow!("indented block to start with indentation"));
};
let indented = format!("{indentation}el{unindented}");

Ok(Fix::safe_edit(Edit::replacement(
indented,
locator.line_start(else_clause.start()),
inner_if_line_end,
)))
Ok(Fix::safe_edits(
Edit::replacement(
indented,
locator.line_start(else_clause.start()),
inner_if_line_end,
),
trivia,
))
}
170 changes: 153 additions & 17 deletions ...s/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR5501_collapsible_else_if.py.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,18 +73,19 @@ collapsible_else_if.py:55:5: PLR5501 [*] Use `elif` instead of `else` then `if`,
52 52 | def not_ok1_with_comments():
53 53 | if 1:
54 54 | pass
55 |+ elif 2:
56 |+ pass
55 57 | else:
55 |+ # inner comment
56 |+ elif 2:
57 |+ pass
55 58 | else:
56 |- # inner comment
57 |- if 2:
58 |- pass
59 |- else:
60 |- pass # final pass comment
58 |+ pass # final pass comment
61 59 |
62 60 |
63 61 | # Regression test for https://github.com/apache/airflow/blob/f1e1cdcc3b2826e68ba133f350300b5065bbca33/airflow/models/dag.py#L1737
59 |+ pass # final pass comment
61 60 |
62 61 |
63 62 | # Regression test for https://github.com/apache/airflow/blob/f1e1cdcc3b2826e68ba133f350300b5065bbca33/airflow/models/dag.py#L1737

collapsible_else_if.py:69:5: PLR5501 [*] Use `elif` instead of `else` then `if`, to reduce indentation
|
Expand Down Expand Up @@ -181,15 +182,150 @@ collapsible_else_if.py:96:5: PLR5501 [*] Use `elif` instead of `else` then `if`,
= help: Convert to `elif`

ℹ Safe fix
93 93 | def not_ok5():
94 94 | if 1:
95 95 | pass
96 |- else:
97 |- if 2:
98 |- pass
99 |- else: pass
96 |+ elif 2:
97 |+ pass
98 |+ else: pass
93 93 | def not_ok5():
94 94 | if 1:
95 95 | pass
96 |- else:
97 |- if 2:
98 |- pass
99 |- else: pass
96 |+ elif 2:
97 |+ pass
98 |+ else: pass
100 99 |
101 100 |
102 101 | def not_ok1_with_multiline_comments():

collapsible_else_if.py:105:5: PLR5501 [*] Use `elif` instead of `else` then `if`, to reduce indentation
|
103 | if 1:
104 | pass
105 | else:
| _____^
106 | | # inner comment which happens
107 | | # to be longer than one line
108 | | if 2:
| |________^ PLR5501
109 | pass
110 | else:
|
= help: Convert to `elif`

ℹ Safe fix
102 102 | def not_ok1_with_multiline_comments():
103 103 | if 1:
104 104 | pass
105 |+ # inner comment which happens
106 |+ # to be longer than one line
107 |+ elif 2:
108 |+ pass
105 109 | else:
106 |- # inner comment which happens
107 |- # to be longer than one line
108 |- if 2:
109 |- pass
110 |- else:
111 |- pass # final pass comment
110 |+ pass # final pass comment
112 111 |
113 112 |
114 113 | def not_ok1_with_deep_indented_comments():

collapsible_else_if.py:117:5: PLR5501 [*] Use `elif` instead of `else` then `if`, to reduce indentation
|
115 | if 1:
116 | pass
117 | else:
| _____^
118 | | # inner comment which happens to be overly indented
119 | | if 2:
| |________^ PLR5501
120 | pass
121 | else:
|
= help: Convert to `elif`

ℹ Safe fix
114 114 | def not_ok1_with_deep_indented_comments():
115 115 | if 1:
116 116 | pass
117 |+ # inner comment which happens to be overly indented
118 |+ elif 2:
119 |+ pass
117 120 | else:
118 |- # inner comment which happens to be overly indented
119 |- if 2:
120 |- pass
121 |- else:
122 |- pass # final pass comment
121 |+ pass # final pass comment
123 122 |
124 123 |
125 124 | def not_ok1_with_shallow_indented_comments():

collapsible_else_if.py:128:5: PLR5501 [*] Use `elif` instead of `else` then `if`, to reduce indentation
|
126 | if 1:
127 | pass
128 | else:
| _____^
129 | | # inner comment which happens to be under indented
130 | | if 2:
| |________^ PLR5501
131 | pass
132 | else:
|
= help: Convert to `elif`

ℹ Safe fix
125 125 | def not_ok1_with_shallow_indented_comments():
126 126 | if 1:
127 127 | pass
128 |- else:
129 128 | # inner comment which happens to be under indented
130 |- if 2:
131 |- pass
132 |- else:
133 |- pass # final pass comment
129 |+ elif 2:
130 |+ pass
131 |+ else:
132 |+ pass # final pass comment
134 133 |
135 134 |
136 135 | def not_ok1_with_mixed_indented_comments():

collapsible_else_if.py:139:5: PLR5501 [*] Use `elif` instead of `else` then `if`, to reduce indentation
|
137 | if 1:
138 | pass
139 | else:
| _____^
140 | | # inner comment which has mixed
141 | | # indentation levels
142 | | # which is pretty weird
143 | | if 2:
| |________^ PLR5501
144 | pass
145 | else:
|
= help: Convert to `elif`

ℹ Safe fix
136 136 | def not_ok1_with_mixed_indented_comments():
137 137 | if 1:
138 138 | pass
139 |+ # inner comment which has mixed
140 |+ # indentation levels
141 |+ # which is pretty weird
142 |+ elif 2:
143 |+ pass
139 144 | else:
140 |- # inner comment which has mixed
141 |- # indentation levels
142 |- # which is pretty weird
143 |- if 2:
144 |- pass
145 |- else:
146 |- pass # final pass comment
145 |+ pass # final pass comment

0 comments on commit cc1f766

Please sign in to comment.