Skip to content

Commit cf0f5e1

Browse files
MichaReiserMichaReiser
authored
Fix formatting of single with-item with trailing comment (#14005)
1 parent 20b8a43 commit cf0f5e1

File tree

5 files changed

+276
-14
lines changed

5 files changed

+276
-14
lines changed

‎crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/with.py‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -328,3 +328,42 @@
328328
if True:
329329
with anyio.CancelScope(shield=True) if get_running_loop() else contextlib.nullcontext() as b:
330330
pass
331+
332+
333+
# Regression test for https://github.com/astral-sh/ruff/issues/14001
334+
with (
335+
open(
336+
"some/path.txt",
337+
"rb",
338+
)
339+
if True
340+
else open("other/path.txt")
341+
# Bar
342+
):
343+
pass
344+
345+
346+
with ( # trailing comment
347+
open(
348+
"some/path.txt",
349+
"rb",
350+
)
351+
if True
352+
else open("other/path.txt")
353+
# Bar
354+
):
355+
pass
356+
357+
358+
with (
359+
(
360+
open(
361+
"some/path.txt",
362+
"rb",
363+
)
364+
if True
365+
else open("other/path.txt")
366+
)
367+
# Bar
368+
):
369+
pass

‎crates/ruff_python_formatter/src/other/with_item.rs‎

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,11 @@ use crate::expression::parentheses::{
66
is_expression_parenthesized, parenthesized, Parentheses, Parenthesize,
77
};
88
use crate::prelude::*;
9-
use crate::preview::is_with_single_item_pre_39_enabled;
9+
use crate::preview::{
10+
is_with_single_item_pre_39_enabled, is_with_single_target_parentheses_enabled,
11+
};
1012

11-
#[derive(Copy, Clone, Debug, Eq, PartialEq, Default)]
13+
#[derive(Copy, Clone, Debug, Eq, PartialEq)]
1214
pub enum WithItemLayout {
1315
/// A with item that is the `with`s only context manager and its context expression is parenthesized.
1416
///
@@ -69,15 +71,20 @@ pub enum WithItemLayout {
6971
/// a
7072
// ): ...
7173
/// ```
72-
#[default]
73-
ParenthesizedContextManagers,
74+
ParenthesizedContextManagers { single: bool },
7475
}
7576

7677
#[derive(Default)]
7778
pub struct FormatWithItem {
7879
layout: WithItemLayout,
7980
}
8081

82+
impl Default for WithItemLayout {
83+
fn default() -> Self {
84+
WithItemLayout::ParenthesizedContextManagers { single: false }
85+
}
86+
}
87+
8188
impl FormatRuleWithOptions<WithItem, PyFormatContext<'_>> for FormatWithItem {
8289
type Options = WithItemLayout;
8390

@@ -97,6 +104,10 @@ impl FormatNodeRule<WithItem> for FormatWithItem {
97104
let comments = f.context().comments().clone();
98105
let trailing_as_comments = comments.dangling(item);
99106

107+
// WARNING: The `is_parenthesized` returns false-positives
108+
// if the `with` has a single item without a target.
109+
// E.g., it returns `true` for `with (a)` even though the parentheses
110+
// belong to the with statement and not the expression but it can't determine that.
100111
let is_parenthesized = is_expression_parenthesized(
101112
context_expr.into(),
102113
f.context().comments().ranges(),
@@ -105,10 +116,17 @@ impl FormatNodeRule<WithItem> for FormatWithItem {
105116

106117
match self.layout {
107118
// Remove the parentheses of the `with_items` if the with statement adds parentheses
108-
WithItemLayout::ParenthesizedContextManagers => {
109-
if is_parenthesized {
110-
// ...except if the with item is parenthesized, then use this with item as a preferred breaking point
111-
// or when it has comments, then parenthesize it to prevent comments from moving.
119+
WithItemLayout::ParenthesizedContextManagers { single } => {
120+
// ...except if the with item is parenthesized and it's not the only with item or it has a target.
121+
// Then use the context expression as a preferred breaking point.
122+
let prefer_breaking_context_expression =
123+
if is_with_single_target_parentheses_enabled(f.context()) {
124+
(optional_vars.is_some() || !single) && is_parenthesized
125+
} else {
126+
is_parenthesized
127+
};
128+
129+
if prefer_breaking_context_expression {
112130
maybe_parenthesize_expression(
113131
context_expr,
114132
item,

‎crates/ruff_python_formatter/src/preview.rs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,3 +72,10 @@ pub(crate) fn is_docstring_code_block_in_docstring_indent_enabled(
7272
pub(crate) fn is_join_implicit_concatenated_string_enabled(context: &PyFormatContext) -> bool {
7373
context.is_preview()
7474
}
75+
76+
/// Returns `true` if the bugfix for single-with items with a trailing comment targeting Python 3.9 or newer is enabled.
77+
///
78+
/// See [#14001](https://github.com/astral-sh/ruff/issues/14001)
79+
pub(crate) fn is_with_single_target_parentheses_enabled(context: &PyFormatContext) -> bool {
80+
context.is_preview()
81+
}

‎crates/ruff_python_formatter/src/statement/stmt_with.rs‎

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,10 @@ impl FormatNodeRule<StmtWith> for FormatStmtWith {
7171

7272
match layout {
7373
WithItemsLayout::SingleWithTarget(single) => {
74-
optional_parentheses(&single.format()).fmt(f)
74+
optional_parentheses(&single.format().with_options(
75+
WithItemLayout::ParenthesizedContextManagers { single: true },
76+
))
77+
.fmt(f)
7578
}
7679

7780
WithItemsLayout::SingleWithoutTarget(single) => single
@@ -93,7 +96,11 @@ impl FormatNodeRule<StmtWith> for FormatStmtWith {
9396
for item in &with_stmt.items {
9497
joiner.entry_with_line_separator(
9598
item,
96-
&item.format(),
99+
&item.format().with_options(
100+
WithItemLayout::ParenthesizedContextManagers {
101+
single: with_stmt.items.len() == 1,
102+
},
103+
),
97104
soft_line_break_or_space(),
98105
);
99106
}
@@ -114,9 +121,22 @@ impl FormatNodeRule<StmtWith> for FormatStmtWith {
114121
WithItemsLayout::Parenthesized => parenthesized(
115122
"(",
116123
&format_with(|f: &mut PyFormatter| {
117-
f.join_comma_separated(with_stmt.body.first().unwrap().start())
118-
.nodes(&with_stmt.items)
119-
.finish()
124+
let mut joiner = f.join_comma_separated(
125+
with_stmt.body.first().unwrap().start(),
126+
);
127+
128+
for item in &with_stmt.items {
129+
joiner.entry(
130+
item,
131+
&item.format().with_options(
132+
WithItemLayout::ParenthesizedContextManagers {
133+
single: with_stmt.items.len() == 1,
134+
},
135+
),
136+
);
137+
}
138+
139+
joiner.finish()
120140
}),
121141
")",
122142
)

‎crates/ruff_python_formatter/tests/snapshots/format@statement__with.py.snap‎

Lines changed: 179 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
---
22
source: crates/ruff_python_formatter/tests/fixtures.rs
33
input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/with.py
4+
snapshot_kind: text
45
---
56
## Input
67
```python
@@ -334,6 +335,45 @@ with aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
334335
if True:
335336
with anyio.CancelScope(shield=True) if get_running_loop() else contextlib.nullcontext() as b:
336337
pass
338+
339+
340+
# Regression test for https://github.com/astral-sh/ruff/issues/14001
341+
with (
342+
open(
343+
"some/path.txt",
344+
"rb",
345+
)
346+
if True
347+
else open("other/path.txt")
348+
# Bar
349+
):
350+
pass
351+
352+
353+
with ( # trailing comment
354+
open(
355+
"some/path.txt",
356+
"rb",
357+
)
358+
if True
359+
else open("other/path.txt")
360+
# Bar
361+
):
362+
pass
363+
364+
365+
with (
366+
(
367+
open(
368+
"some/path.txt",
369+
"rb",
370+
)
371+
if True
372+
else open("other/path.txt")
373+
)
374+
# Bar
375+
):
376+
pass
337377
```
338378

339379
## Outputs
@@ -710,6 +750,49 @@ if True:
710750
shield=True
711751
) if get_running_loop() else contextlib.nullcontext() as b:
712752
pass
753+
754+
755+
# Regression test for https://github.com/astral-sh/ruff/issues/14001
756+
with (
757+
(
758+
open(
759+
"some/path.txt",
760+
"rb",
761+
)
762+
if True
763+
else open("other/path.txt")
764+
)
765+
# Bar
766+
):
767+
pass
768+
769+
770+
with ( # trailing comment
771+
(
772+
open(
773+
"some/path.txt",
774+
"rb",
775+
)
776+
if True
777+
else open("other/path.txt")
778+
)
779+
# Bar
780+
):
781+
pass
782+
783+
784+
with (
785+
(
786+
open(
787+
"some/path.txt",
788+
"rb",
789+
)
790+
if True
791+
else open("other/path.txt")
792+
)
793+
# Bar
794+
):
795+
pass
713796
```
714797

715798

@@ -787,7 +870,7 @@ if True:
787870
pass
788871
789872
with Child(aaaaaaaaa, bbbbbbbbbbbbbbb, cccccc), Document(
790-
@@ -344,14 +356,20 @@
873+
@@ -344,57 +356,57 @@
791874
):
792875
pass
793876
@@ -813,6 +896,64 @@ if True:
813896
+ else contextlib.nullcontext()
814897
+ ) as b:
815898
pass
899+
900+
901+
# Regression test for https://github.com/astral-sh/ruff/issues/14001
902+
with (
903+
- (
904+
- open(
905+
- "some/path.txt",
906+
- "rb",
907+
- )
908+
- if True
909+
- else open("other/path.txt")
910+
+ open(
911+
+ "some/path.txt",
912+
+ "rb",
913+
)
914+
+ if True
915+
+ else open("other/path.txt")
916+
# Bar
917+
):
918+
pass
919+
920+
921+
with ( # trailing comment
922+
- (
923+
- open(
924+
- "some/path.txt",
925+
- "rb",
926+
- )
927+
- if True
928+
- else open("other/path.txt")
929+
+ open(
930+
+ "some/path.txt",
931+
+ "rb",
932+
)
933+
+ if True
934+
+ else open("other/path.txt")
935+
# Bar
936+
):
937+
pass
938+
939+
940+
with (
941+
- (
942+
- open(
943+
- "some/path.txt",
944+
- "rb",
945+
- )
946+
- if True
947+
- else open("other/path.txt")
948+
+ open(
949+
+ "some/path.txt",
950+
+ "rb",
951+
)
952+
+ if True
953+
+ else open("other/path.txt")
954+
# Bar
955+
):
956+
pass
816957
```
817958

818959

@@ -1237,4 +1378,41 @@ if True:
12371378
else contextlib.nullcontext() as b
12381379
):
12391380
pass
1381+
1382+
1383+
# Regression test for https://github.com/astral-sh/ruff/issues/14001
1384+
with (
1385+
open(
1386+
"some/path.txt",
1387+
"rb",
1388+
)
1389+
if True
1390+
else open("other/path.txt")
1391+
# Bar
1392+
):
1393+
pass
1394+
1395+
1396+
with ( # trailing comment
1397+
open(
1398+
"some/path.txt",
1399+
"rb",
1400+
)
1401+
if True
1402+
else open("other/path.txt")
1403+
# Bar
1404+
):
1405+
pass
1406+
1407+
1408+
with (
1409+
open(
1410+
"some/path.txt",
1411+
"rb",
1412+
)
1413+
if True
1414+
else open("other/path.txt")
1415+
# Bar
1416+
):
1417+
pass
12401418
```

0 commit comments

Comments
 (0)