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

Jump to bottom

Parenthesize single-generator arguments when adding reverse keyword #7365

Merged
merged 1 commit into from
Sep 13, 2023
Merged

Parenthesize single-generator arguments when adding reverse keyword #7365

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
5 changes: 5 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_comprehensions/C413.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@
reversed(sorted(x, reverse=True, key=lambda e: e))
reversed(sorted(x, reverse=False))

# Regression test for: https://github.com/astral-sh/ruff/issues/7289
reversed(sorted(i for i in range(42)))
reversed(sorted((i for i in range(42)), reverse=True))


def reversed(*args, **kwargs):
return None

Expand Down
24 changes: 23 additions & 1 deletion crates/ruff/src/rules/flake8_comprehensions/fixes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -718,7 +718,7 @@ pub(crate) fn fix_unnecessary_call_around_sorted(
if outer_name.value == "list" {
tree = Expression::Call(Box::new((*inner_call).clone()));
} else {
// If the `reverse` argument is used
// If the `reverse` argument is used...
let args = if inner_call.args.iter().any(|arg| {
matches!(
arg.keyword,
Expand Down Expand Up @@ -770,6 +770,28 @@ pub(crate) fn fix_unnecessary_call_around_sorted(
.collect_vec()
} else {
let mut args = inner_call.args.clone();

// If necessary, parenthesize a generator expression, as a generator expression must
// be parenthesized if it's not a solitary argument. For example, given:
// ```python
// reversed(sorted(i for i in range(42)))
// ```
// Rewrite as:
// ```python
// sorted((i for i in range(42)), reverse=True)
// ```
if let [arg] = args.as_mut_slice() {
if matches!(arg.value, Expression::GeneratorExp(_)) {
if arg.value.lpar().is_empty() && arg.value.rpar().is_empty() {
arg.value = arg
.value
.clone()
.with_parens(LeftParen::default(), RightParen::default());
}
}
}

// Add the `reverse=True` argument.
args.push(Arg {
value: Expression::Name(Box::new(Name {
value: "True",
Expand Down
46 changes: 42 additions & 4 deletions ...ke8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C413_C413.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ C413.py:8:1: C413 [*] Unnecessary `reversed` call around `sorted()`
8 |+sorted(x, reverse=False, key=lambda e: e)
9 9 | reversed(sorted(x, reverse=False))
10 10 |
11 11 | def reversed(*args, **kwargs):
11 11 | # Regression test for: https://github.com/astral-sh/ruff/issues/7289

C413.py:9:1: C413 [*] Unnecessary `reversed` call around `sorted()`
|
Expand All @@ -132,7 +132,7 @@ C413.py:9:1: C413 [*] Unnecessary `reversed` call around `sorted()`
9 | reversed(sorted(x, reverse=False))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C413
10 |
11 | def reversed(*args, **kwargs):
11 | # Regression test for: https://github.com/astral-sh/ruff/issues/7289
|
= help: Remove unnecessary `reversed` call

Expand All @@ -143,7 +143,45 @@ C413.py:9:1: C413 [*] Unnecessary `reversed` call around `sorted()`
9 |-reversed(sorted(x, reverse=False))
9 |+sorted(x, reverse=True)
10 10 |
11 11 | def reversed(*args, **kwargs):
12 12 | return None
11 11 | # Regression test for: https://github.com/astral-sh/ruff/issues/7289
12 12 | reversed(sorted(i for i in range(42)))

C413.py:12:1: C413 [*] Unnecessary `reversed` call around `sorted()`
|
11 | # Regression test for: https://github.com/astral-sh/ruff/issues/7289
12 | reversed(sorted(i for i in range(42)))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C413
13 | reversed(sorted((i for i in range(42)), reverse=True))
|
= help: Remove unnecessary `reversed` call

ℹ Suggested fix
9 9 | reversed(sorted(x, reverse=False))
10 10 |
11 11 | # Regression test for: https://github.com/astral-sh/ruff/issues/7289
12 |-reversed(sorted(i for i in range(42)))
12 |+sorted((i for i in range(42)), reverse=True)
13 13 | reversed(sorted((i for i in range(42)), reverse=True))
14 14 |
15 15 |

C413.py:13:1: C413 [*] Unnecessary `reversed` call around `sorted()`
|
11 | # Regression test for: https://github.com/astral-sh/ruff/issues/7289
12 | reversed(sorted(i for i in range(42)))
13 | reversed(sorted((i for i in range(42)), reverse=True))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C413
|
= help: Remove unnecessary `reversed` call

ℹ Suggested fix
10 10 |
11 11 | # Regression test for: https://github.com/astral-sh/ruff/issues/7289
12 12 | reversed(sorted(i for i in range(42)))
13 |-reversed(sorted((i for i in range(42)), reverse=True))
13 |+sorted((i for i in range(42)), reverse=False)
14 14 |
15 15 |
16 16 | def reversed(*args, **kwargs):


Loading