Skip to content

Commit 136abac

Browse files
hamirmahalntBre
andauthored
[flake8-logging-format] Add auto-fix for f-string logging calls (G004) (#19303)
Closes #19302 <!-- Thank you for contributing to Ruff/ty! To help us out with reviewing, please consider the following: - Does this pull request include a summary of the change? (See below.) - Does this pull request include a descriptive title? (Please prefix with `[ty]` for ty pull requests.) - Does this pull request include references to any relevant issues? --> ## Summary This adds an auto-fix for `Logging statement uses f-string` Ruff G004, so users don't have to resolve it manually. <!-- What's the purpose of the change? What does it do, and why? --> ## Test Plan I ran the auto-fixes on a Python file locally and and it worked as expected. <!-- How was it tested? --> --------- Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com>
1 parent bc7274d commit 136abac

10 files changed

+674
-5
lines changed

crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G004.py

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,3 +17,50 @@
1717
# Don't trigger for t-strings
1818
info(t"{name}")
1919
info(t"{__name__}")
20+
21+
count = 5
22+
total = 9
23+
directory_path = "/home/hamir/ruff/crates/ruff_linter/resources/test/"
24+
logging.info(f"{count} out of {total} files in {directory_path} checked")
25+
26+
27+
28+
x = 99
29+
fmt = "08d"
30+
logger.info(f"{x:{'08d'}}")
31+
logger.info(f"{x:>10} {x:{fmt}}")
32+
33+
logging.info(f"")
34+
logging.info(f"This message doesn't have any variables.")
35+
36+
obj = {"key": "value"}
37+
logging.info(f"Object: {obj!r}")
38+
39+
items_count = 3
40+
logging.warning(f"Items: {items_count:d}")
41+
42+
data = {"status": "active"}
43+
logging.info(f"Processing {len(data)} items")
44+
logging.info(f"Status: {data.get('status', 'unknown').upper()}")
45+
46+
47+
result = 123
48+
logging.info(f"Calculated result: {result + 100}")
49+
50+
temperature = 123
51+
logging.info(f"Temperature: {temperature:.1f}°C")
52+
53+
class FilePath:
54+
def __init__(self, name: str):
55+
self.name = name
56+
57+
logging.info(f"No changes made to {file_path.name}.")
58+
59+
user = "tron"
60+
balance = 123.45
61+
logging.error(f"Error {404}: User {user} has insufficient balance ${balance:.2f}")
62+
63+
import logging
64+
65+
x = 1
66+
logging.error(f"{x} -> %s", x)
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
"""Test f-string argument order."""
2+
3+
import logging
4+
5+
logger = logging.getLogger(__name__)
6+
7+
X = 1
8+
Y = 2
9+
logger.error(f"{X} -> %s", Y)
10+
logger.error(f"{Y} -> %s", X)

crates/ruff_linter/src/preview.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,11 @@ pub(crate) const fn is_bad_version_info_in_non_stub_enabled(settings: &LinterSet
4040
settings.preview.is_enabled()
4141
}
4242

43+
/// <https://github.com/astral-sh/ruff/pull/19303>
44+
pub(crate) const fn is_fix_f_string_logging_enabled(settings: &LinterSettings) -> bool {
45+
settings.preview.is_enabled()
46+
}
47+
4348
// https://github.com/astral-sh/ruff/pull/16719
4449
pub(crate) const fn is_fix_manual_dict_comprehension_enabled(settings: &LinterSettings) -> bool {
4550
settings.preview.is_enabled()

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ mod tests {
2222
#[test_case(Path::new("G002.py"))]
2323
#[test_case(Path::new("G003.py"))]
2424
#[test_case(Path::new("G004.py"))]
25+
#[test_case(Path::new("G004_arg_order.py"))]
2526
#[test_case(Path::new("G010.py"))]
2627
#[test_case(Path::new("G101_1.py"))]
2728
#[test_case(Path::new("G101_2.py"))]
@@ -48,4 +49,24 @@ mod tests {
4849
assert_diagnostics!(snapshot, diagnostics);
4950
Ok(())
5051
}
52+
53+
#[test_case(Rule::LoggingFString, Path::new("G004.py"))]
54+
#[test_case(Rule::LoggingFString, Path::new("G004_arg_order.py"))]
55+
fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
56+
let snapshot = format!(
57+
"preview__{}_{}",
58+
rule_code.noqa_code(),
59+
path.to_string_lossy()
60+
);
61+
let diagnostics = test_path(
62+
Path::new("flake8_logging_format").join(path).as_path(),
63+
&settings::LinterSettings {
64+
logger_objects: vec!["logging_setup.logger".to_string()],
65+
preview: settings::types::PreviewMode::Enabled,
66+
..settings::LinterSettings::for_rule(rule_code)
67+
},
68+
)?;
69+
assert_diagnostics!(snapshot, diagnostics);
70+
Ok(())
71+
}
5172
}

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

Lines changed: 90 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,99 @@
1-
use ruff_python_ast::{self as ast, Arguments, Expr, Keyword, Operator};
1+
use ruff_python_ast::InterpolatedStringElement;
2+
use ruff_python_ast::{self as ast, Arguments, Expr, Keyword, Operator, StringFlags};
23
use ruff_python_semantic::analyze::logging;
34
use ruff_python_stdlib::logging::LoggingLevel;
45
use ruff_text_size::Ranged;
56

67
use crate::checkers::ast::Checker;
8+
use crate::preview::is_fix_f_string_logging_enabled;
79
use crate::registry::Rule;
810
use crate::rules::flake8_logging_format::violations::{
911
LoggingExcInfo, LoggingExtraAttrClash, LoggingFString, LoggingPercentFormat,
1012
LoggingRedundantExcInfo, LoggingStringConcat, LoggingStringFormat, LoggingWarn,
1113
};
1214
use crate::{Edit, Fix};
1315

16+
fn logging_f_string(
17+
checker: &Checker,
18+
msg: &Expr,
19+
f_string: &ast::ExprFString,
20+
arguments: &Arguments,
21+
msg_pos: usize,
22+
) {
23+
// Report the diagnostic up-front so we can attach a fix later only when preview is enabled.
24+
let mut diagnostic = checker.report_diagnostic(LoggingFString, msg.range());
25+
26+
// Preview gate for the automatic fix.
27+
if !is_fix_f_string_logging_enabled(checker.settings()) {
28+
return;
29+
}
30+
31+
// If there are existing positional arguments after the message, bail out.
32+
// This could indicate a mistake or complex usage we shouldn't try to fix.
33+
if arguments.args.len() > msg_pos + 1 {
34+
return;
35+
}
36+
37+
let mut format_string = String::new();
38+
let mut args: Vec<&str> = Vec::new();
39+
40+
// Try to reuse the first part's quote style when building the replacement.
41+
// Default to double quotes if we can't determine it.
42+
let quote_str = f_string
43+
.value
44+
.f_strings()
45+
.next()
46+
.map(|f| f.flags.quote_str())
47+
.unwrap_or("\"");
48+
49+
for f in f_string.value.f_strings() {
50+
for element in &f.elements {
51+
match element {
52+
InterpolatedStringElement::Literal(lit) => {
53+
// If the literal text contains a '%' placeholder, bail out: mixing
54+
// f-string interpolation with '%' placeholders is ambiguous for our
55+
// automatic conversion, so don't offer a fix for this case.
56+
if lit.value.as_ref().contains('%') {
57+
return;
58+
}
59+
format_string.push_str(lit.value.as_ref());
60+
}
61+
InterpolatedStringElement::Interpolation(interpolated) => {
62+
if interpolated.format_spec.is_some()
63+
|| !matches!(
64+
interpolated.conversion,
65+
ruff_python_ast::ConversionFlag::None
66+
)
67+
{
68+
return;
69+
}
70+
match interpolated.expression.as_ref() {
71+
Expr::Name(name) => {
72+
format_string.push_str("%s");
73+
args.push(name.id.as_str());
74+
}
75+
_ => return,
76+
}
77+
}
78+
}
79+
}
80+
}
81+
82+
if args.is_empty() {
83+
return;
84+
}
85+
86+
let replacement = format!(
87+
"{q}{format_string}{q}, {args}",
88+
q = quote_str,
89+
format_string = format_string,
90+
args = args.join(", ")
91+
);
92+
93+
let fix = Fix::safe_edit(Edit::range_replacement(replacement, msg.range()));
94+
diagnostic.set_fix(fix);
95+
}
96+
1497
/// Returns `true` if the attribute is a reserved attribute on the `logging` module's `LogRecord`
1598
/// class.
1699
fn is_reserved_attr(attr: &str) -> bool {
@@ -42,7 +125,7 @@ fn is_reserved_attr(attr: &str) -> bool {
42125
}
43126

44127
/// Check logging messages for violations.
45-
fn check_msg(checker: &Checker, msg: &Expr) {
128+
fn check_msg(checker: &Checker, msg: &Expr, arguments: &Arguments, msg_pos: usize) {
46129
match msg {
47130
// Check for string concatenation and percent format.
48131
Expr::BinOp(ast::ExprBinOp { op, .. }) => match op {
@@ -55,8 +138,10 @@ fn check_msg(checker: &Checker, msg: &Expr) {
55138
_ => {}
56139
},
57140
// Check for f-strings.
58-
Expr::FString(_) => {
59-
checker.report_diagnostic_if_enabled(LoggingFString, msg.range());
141+
Expr::FString(f_string) => {
142+
if checker.is_rule_enabled(Rule::LoggingFString) {
143+
logging_f_string(checker, msg, f_string, arguments, msg_pos);
144+
}
60145
}
61146
// Check for .format() calls.
62147
Expr::Call(ast::ExprCall { func, .. }) => {
@@ -168,7 +253,7 @@ pub(crate) fn logging_call(checker: &Checker, call: &ast::ExprCall) {
168253
// G001, G002, G003, G004
169254
let msg_pos = usize::from(matches!(logging_call_type, LoggingCallType::LogCall));
170255
if let Some(format_arg) = call.arguments.find_argument_value("msg", msg_pos) {
171-
check_msg(checker, format_arg);
256+
check_msg(checker, format_arg, &call.arguments, msg_pos);
172257
}
173258

174259
// G010

0 commit comments

Comments
 (0)