Skip to content

Commit 66885e4

Browse files
authored
[flake8-logging-format] Avoid dropping implicitly concatenated pieces in the G004 fix (#20793)
## Summary The original autofix for G004 was quietly dropping everything but the f-string components of any implicit concatenation sequence; this addresses that. Side note: It looks like `f_strings` is a bit risky to use (since it implicitly skips non-f-string parts); use iter and include implicitly concatenated pieces. We should consider if it's worth having (convenience vs. bit risky). ## Test Plan ``` cargo test -p ruff_linter ``` Backtest (run new testcases against previous implementation): ``` git checkout HEAD^ crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs cargot test -p ruff_linter ```
1 parent 8248193 commit 66885e4

File tree

5 files changed

+139
-27
lines changed

5 files changed

+139
-27
lines changed
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
import logging
2+
3+
variablename = "value"
4+
5+
log = logging.getLogger(__name__)
6+
log.info(f"a" f"b {variablename}")
7+
log.info("a " f"b {variablename}")
8+
log.info("prefix " f"middle {variablename}" f" suffix")

crates/ruff_linter/src/rules/flake8_logging_format/mod.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ mod tests {
2323
#[test_case(Path::new("G003.py"))]
2424
#[test_case(Path::new("G004.py"))]
2525
#[test_case(Path::new("G004_arg_order.py"))]
26+
#[test_case(Path::new("G004_implicit_concat.py"))]
2627
#[test_case(Path::new("G010.py"))]
2728
#[test_case(Path::new("G101_1.py"))]
2829
#[test_case(Path::new("G101_2.py"))]
@@ -52,6 +53,7 @@ mod tests {
5253

5354
#[test_case(Rule::LoggingFString, Path::new("G004.py"))]
5455
#[test_case(Rule::LoggingFString, Path::new("G004_arg_order.py"))]
56+
#[test_case(Rule::LoggingFString, Path::new("G004_implicit_concat.py"))]
5557
fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
5658
let snapshot = format!(
5759
"preview__{}_{}",

crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs

Lines changed: 41 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -42,38 +42,52 @@ fn logging_f_string(
4242
// Default to double quotes if we can't determine it.
4343
let quote_str = f_string
4444
.value
45-
.f_strings()
45+
.iter()
46+
.map(|part| match part {
47+
ast::FStringPart::Literal(literal) => literal.flags.quote_str(),
48+
ast::FStringPart::FString(f) => f.flags.quote_str(),
49+
})
4650
.next()
47-
.map(|f| f.flags.quote_str())
4851
.unwrap_or("\"");
4952

50-
for f in f_string.value.f_strings() {
51-
for element in &f.elements {
52-
match element {
53-
InterpolatedStringElement::Literal(lit) => {
54-
// If the literal text contains a '%' placeholder, bail out: mixing
55-
// f-string interpolation with '%' placeholders is ambiguous for our
56-
// automatic conversion, so don't offer a fix for this case.
57-
if lit.value.as_ref().contains('%') {
58-
return;
59-
}
60-
format_string.push_str(lit.value.as_ref());
53+
for part in &f_string.value {
54+
match part {
55+
ast::FStringPart::Literal(literal) => {
56+
let literal_text = literal.as_str();
57+
if literal_text.contains('%') {
58+
return;
6159
}
62-
InterpolatedStringElement::Interpolation(interpolated) => {
63-
if interpolated.format_spec.is_some()
64-
|| !matches!(
65-
interpolated.conversion,
66-
ruff_python_ast::ConversionFlag::None
67-
)
68-
{
69-
return;
70-
}
71-
match interpolated.expression.as_ref() {
72-
Expr::Name(name) => {
73-
format_string.push_str("%s");
74-
args.push(name.id.as_str());
60+
format_string.push_str(literal_text);
61+
}
62+
ast::FStringPart::FString(f) => {
63+
for element in &f.elements {
64+
match element {
65+
InterpolatedStringElement::Literal(lit) => {
66+
// If the literal text contains a '%' placeholder, bail out: mixing
67+
// f-string interpolation with '%' placeholders is ambiguous for our
68+
// automatic conversion, so don't offer a fix for this case.
69+
if lit.value.as_ref().contains('%') {
70+
return;
71+
}
72+
format_string.push_str(lit.value.as_ref());
73+
}
74+
InterpolatedStringElement::Interpolation(interpolated) => {
75+
if interpolated.format_spec.is_some()
76+
|| !matches!(
77+
interpolated.conversion,
78+
ruff_python_ast::ConversionFlag::None
79+
)
80+
{
81+
return;
82+
}
83+
match interpolated.expression.as_ref() {
84+
Expr::Name(name) => {
85+
format_string.push_str("%s");
86+
args.push(name.id.as_str());
87+
}
88+
_ => return,
89+
}
7590
}
76-
_ => return,
7791
}
7892
}
7993
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
---
2+
source: crates/ruff_linter/src/rules/flake8_logging_format/mod.rs
3+
assertion_line: 50
4+
---
5+
G004 Logging statement uses f-string
6+
--> G004_implicit_concat.py:6:10
7+
|
8+
5 | log = logging.getLogger(__name__)
9+
6 | log.info(f"a" f"b {variablename}")
10+
| ^^^^^^^^^^^^^^^^^^^^^^^^
11+
7 | log.info("a " f"b {variablename}")
12+
8 | log.info("prefix " f"middle {variablename}" f" suffix")
13+
|
14+
help: Convert to lazy `%` formatting
15+
16+
G004 Logging statement uses f-string
17+
--> G004_implicit_concat.py:7:10
18+
|
19+
5 | log = logging.getLogger(__name__)
20+
6 | log.info(f"a" f"b {variablename}")
21+
7 | log.info("a " f"b {variablename}")
22+
| ^^^^^^^^^^^^^^^^^^^^^^^^
23+
8 | log.info("prefix " f"middle {variablename}" f" suffix")
24+
|
25+
help: Convert to lazy `%` formatting
26+
27+
G004 Logging statement uses f-string
28+
--> G004_implicit_concat.py:8:10
29+
|
30+
6 | log.info(f"a" f"b {variablename}")
31+
7 | log.info("a " f"b {variablename}")
32+
8 | log.info("prefix " f"middle {variablename}" f" suffix")
33+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
34+
|
35+
help: Convert to lazy `%` formatting
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
---
2+
source: crates/ruff_linter/src/rules/flake8_logging_format/mod.rs
3+
assertion_line: 71
4+
---
5+
G004 [*] Logging statement uses f-string
6+
--> G004_implicit_concat.py:6:10
7+
|
8+
5 | log = logging.getLogger(__name__)
9+
6 | log.info(f"a" f"b {variablename}")
10+
| ^^^^^^^^^^^^^^^^^^^^^^^^
11+
7 | log.info("a " f"b {variablename}")
12+
8 | log.info("prefix " f"middle {variablename}" f" suffix")
13+
|
14+
help: Convert to lazy `%` formatting
15+
3 | variablename = "value"
16+
4 |
17+
5 | log = logging.getLogger(__name__)
18+
- log.info(f"a" f"b {variablename}")
19+
6 + log.info("ab %s", variablename)
20+
7 | log.info("a " f"b {variablename}")
21+
8 | log.info("prefix " f"middle {variablename}" f" suffix")
22+
23+
G004 [*] Logging statement uses f-string
24+
--> G004_implicit_concat.py:7:10
25+
|
26+
5 | log = logging.getLogger(__name__)
27+
6 | log.info(f"a" f"b {variablename}")
28+
7 | log.info("a " f"b {variablename}")
29+
| ^^^^^^^^^^^^^^^^^^^^^^^^
30+
8 | log.info("prefix " f"middle {variablename}" f" suffix")
31+
|
32+
help: Convert to lazy `%` formatting
33+
4 |
34+
5 | log = logging.getLogger(__name__)
35+
6 | log.info(f"a" f"b {variablename}")
36+
- log.info("a " f"b {variablename}")
37+
7 + log.info("a b %s", variablename)
38+
8 | log.info("prefix " f"middle {variablename}" f" suffix")
39+
40+
G004 [*] Logging statement uses f-string
41+
--> G004_implicit_concat.py:8:10
42+
|
43+
6 | log.info(f"a" f"b {variablename}")
44+
7 | log.info("a " f"b {variablename}")
45+
8 | log.info("prefix " f"middle {variablename}" f" suffix")
46+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
47+
|
48+
help: Convert to lazy `%` formatting
49+
5 | log = logging.getLogger(__name__)
50+
6 | log.info(f"a" f"b {variablename}")
51+
7 | log.info("a " f"b {variablename}")
52+
- log.info("prefix " f"middle {variablename}" f" suffix")
53+
8 + log.info("prefix middle %s suffix", variablename)

0 commit comments

Comments
 (0)