Skip to content
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.

Sign up for GitHub

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

Preserve trivia (i.e. comments) in PLR5501 #13573

Merged
merged 1 commit into from
Oct 3, 2024
+225 −23
Merged

Preserve trivia (i.e. comments) in PLR5501 #13573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pylint/collapsible_else_if.py
View file
Open in desktop
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
Open in desktop
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 {
Comment on lines +129 to +132
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is trivia_range ever going to be empty? Because there should at least be an indentation between the else: line end and the start of the first statement.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question.. uhh let me look.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put a debug statement in there and the trivia range is indeed empty when there's no trivia 🤷‍♀️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think it might change when the linter is error resilient but that doesn't need to be addressed now

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,
))
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved
}
170 changes: 153 additions & 17 deletions ...s/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR5501_collapsible_else_if.py.snap
View file
Open in desktop
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
Loading