From ec17da886891173703cca78044fb188e8271a960 Mon Sep 17 00:00:00 2001 From: Hamir Mahal Date: Sun, 24 Aug 2025 13:19:06 -0700 Subject: [PATCH 1/6] feat: add auto-fix for f-string logging statements --- .../fixtures/flake8_logging_format/G004.py | 47 ++++++ .../flake8_logging_format/G004_arg_order.py | 10 ++ crates/ruff_linter/src/preview.rs | 5 + .../src/rules/flake8_logging_format/mod.rs | 39 +++++ .../rules/logging_call.rs | 106 +++++++++++- ...flake8_logging_format__tests__G004.py.snap | 158 ++++++++++++++++++ ...ging_format__tests__G004_arg_order.py.snap | 23 +++ .../rules/flake8_logging_format/violations.rs | 6 + 8 files changed, 389 insertions(+), 5 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G004_arg_order.py create mode 100644 crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G004_arg_order.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G004.py b/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G004.py index da05aba630efd..fcfa953a329f9 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G004.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G004.py @@ -17,3 +17,50 @@ # Don't trigger for t-strings info(t"{name}") info(t"{__name__}") + +count = 5 +total = 9 +directory_path = "/home/hamir/ruff/crates/ruff_linter/resources/test/" +logging.info(f"{count} out of {total} files in {directory_path} checked") + + + +x = 99 +fmt = "08d" +logger.info(f"{x:{'08d'}}") +logger.info(f"{x:>10} {x:{fmt}}") + +logging.info(f"") +logging.info(f"This message doesn't have any variables.") + +obj = {"key": "value"} +logging.info(f"Object: {obj!r}") + +items_count = 3 +logging.warning(f"Items: {items_count:d}") + +data = {"status": "active"} +logging.info(f"Processing {len(data)} items") +logging.info(f"Status: {data.get('status', 'unknown').upper()}") + + +result = 123 +logging.info(f"Calculated result: {result + 100}") + +temperature = 123 +logging.info(f"Temperature: {temperature:.1f}°C") + +class FilePath: + def __init__(self, name: str): + self.name = name + +logging.info(f"No changes made to {file_path.name}.") + +user = "tron" +balance = 123.45 +logging.error(f"Error {404}: User {user} has insufficient balance ${balance:.2f}") + +import logging + +x = 1 +logging.error(f"{x} -> %s", x) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G004_arg_order.py b/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G004_arg_order.py new file mode 100644 index 0000000000000..44c370bd3d3b8 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_logging_format/G004_arg_order.py @@ -0,0 +1,10 @@ +"""Test f-string argument order.""" + +import logging + +logger = logging.getLogger(__name__) + +X = 1 +Y = 2 +logger.error(f"{X} -> %s", Y) +logger.error(f"{Y} -> %s", X) diff --git a/crates/ruff_linter/src/preview.rs b/crates/ruff_linter/src/preview.rs index b7d3b5df7abce..6b6a4d15e5e7f 100644 --- a/crates/ruff_linter/src/preview.rs +++ b/crates/ruff_linter/src/preview.rs @@ -40,6 +40,11 @@ pub(crate) const fn is_bad_version_info_in_non_stub_enabled(settings: &LinterSet settings.preview.is_enabled() } +/// +pub(crate) const fn is_fix_f_string_logging_enabled(settings: &LinterSettings) -> bool { + settings.preview.is_enabled() +} + // https://github.com/astral-sh/ruff/pull/16719 pub(crate) const fn is_fix_manual_dict_comprehension_enabled(settings: &LinterSettings) -> bool { settings.preview.is_enabled() diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/mod.rs b/crates/ruff_linter/src/rules/flake8_logging_format/mod.rs index 1dc2b536a629f..940c0df57c48e 100644 --- a/crates/ruff_linter/src/rules/flake8_logging_format/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_logging_format/mod.rs @@ -48,4 +48,43 @@ mod tests { assert_diagnostics!(snapshot, diagnostics); Ok(()) } + + #[test] + fn g004_fix_gated_by_preview_flag() -> Result<()> { + use crate::settings::LinterSettings; + + let diagnostics = test_path( + Path::new("flake8_logging_format") + .join(Path::new("G004_arg_order.py")) + .as_path(), + &LinterSettings { + preview: crate::settings::types::PreviewMode::Disabled, + ..LinterSettings::for_rules(vec![Rule::LoggingFString]) + }, + )?; + + // When preview is disabled, diagnostics should be emitted but no fix should be attached. + assert!(diagnostics.iter().all(|d| d.fix().is_none())); + Ok(()) + } + + #[test] + fn g004_preserves_argument_order_in_fix() -> Result<()> { + use crate::settings::LinterSettings; + + // Use the fixture that contains a two-argument example to ensure ordering is preserved. + let diagnostics = test_path( + Path::new("flake8_logging_format") + .join(Path::new("G004_arg_order.py")) + .as_path(), + &LinterSettings { + preview: crate::settings::types::PreviewMode::Enabled, + ..LinterSettings::for_rules(vec![Rule::LoggingFString]) + }, + )?; + + // Assert by snapshot (disabled/enabled snapshots stored in snapshots/). + assert_diagnostics!("G004_arg_order.py".to_string(), diagnostics); + Ok(()) + } } diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs b/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs index 33727411543c2..225887dc48fac 100644 --- a/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs +++ b/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs @@ -1,15 +1,109 @@ -use ruff_python_ast::{self as ast, Arguments, Expr, Keyword, Operator}; +use ruff_python_ast::InterpolatedStringElement; +use ruff_python_ast::{self as ast, Arguments, Expr, Keyword, Operator, StringFlags}; use ruff_python_semantic::analyze::logging; use ruff_python_stdlib::logging::LoggingLevel; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; +use crate::preview::is_fix_f_string_logging_enabled; use crate::registry::Rule; use crate::rules::flake8_logging_format::violations::{ LoggingExcInfo, LoggingExtraAttrClash, LoggingFString, LoggingPercentFormat, LoggingRedundantExcInfo, LoggingStringConcat, LoggingStringFormat, LoggingWarn, }; use crate::{Edit, Fix}; +fn logging_f_string( + checker: &Checker, + msg: &Expr, + f_string: &ast::ExprFString, + arguments: &Arguments, + msg_pos: usize, +) { + // Report the diagnostic up-front so we can attach a fix later only when preview is enabled. + let mut diagnostic = checker.report_diagnostic(LoggingFString, msg.range()); + + // Preview gate for the automatic fix. + if !is_fix_f_string_logging_enabled(checker.settings()) { + return; + } + + let mut format_string = String::new(); + let mut args = Vec::new(); + + // Try to reuse the first part's quote style when building the replacement. + // Default to double quotes if we can't determine it. + let quote_str = f_string + .value + .f_strings() + .next() + .map(|f| f.flags.quote_str()) + .unwrap_or("\"") + .to_string(); + + for f in f_string.value.f_strings() { + for element in &f.elements { + match element { + InterpolatedStringElement::Literal(lit) => { + // If the literal text contains a '%' placeholder, bail out: mixing + // f-string interpolation with '%' placeholders is ambiguous for our + // automatic conversion, so don't offer a fix for this case. + if lit.value.as_ref().contains('%') { + return; + } + format_string.push_str(lit.value.as_ref()); + } + InterpolatedStringElement::Interpolation(interpolated) => { + if interpolated.format_spec.is_some() + || !matches!( + interpolated.conversion, + ruff_python_ast::ConversionFlag::None + ) + { + return; + } + match interpolated.expression.as_ref() { + Expr::Name(name) => { + format_string.push_str("%s"); + args.push(name.id.to_string()); + } + _ => return, + } + } + } + } + } + + if args.is_empty() { + return; + } + + // Determine names of existing trailing positional args (after the `msg` argument). + let existing_names: Vec = arguments + .args + .iter() + .skip(msg_pos + 1) + .filter_map(|arg| match arg { + Expr::Name(name) => Some(name.id.to_string()), + _ => None, + }) + .collect(); + + // Combine collected interpolation names with existing trailing positional names in + // that order so that placeholders map to arguments correctly. Do not deduplicate; + // duplicates are valid when the same name appears multiple times. + let mut result_args = args.clone(); + result_args.extend(existing_names); + + let replacement = format!( + "{q}{format_string}{q}, {args}", + q = quote_str, + format_string = format_string, + args = result_args.join(", ") + ); + + let fix = Fix::safe_edit(Edit::range_replacement(replacement, msg.range())); + diagnostic.set_fix(fix); +} /// Returns `true` if the attribute is a reserved attribute on the `logging` module's `LogRecord` /// class. @@ -42,7 +136,7 @@ fn is_reserved_attr(attr: &str) -> bool { } /// Check logging messages for violations. -fn check_msg(checker: &Checker, msg: &Expr) { +fn check_msg(checker: &Checker, msg: &Expr, arguments: &Arguments, msg_pos: usize) { match msg { // Check for string concatenation and percent format. Expr::BinOp(ast::ExprBinOp { op, .. }) => match op { @@ -55,8 +149,10 @@ fn check_msg(checker: &Checker, msg: &Expr) { _ => {} }, // Check for f-strings. - Expr::FString(_) => { - checker.report_diagnostic_if_enabled(LoggingFString, msg.range()); + Expr::FString(f_string) => { + if checker.is_rule_enabled(Rule::LoggingFString) { + logging_f_string(checker, msg, f_string, arguments, msg_pos); + } } // Check for .format() calls. Expr::Call(ast::ExprCall { func, .. }) => { @@ -168,7 +264,7 @@ pub(crate) fn logging_call(checker: &Checker, call: &ast::ExprCall) { // G001, G002, G003, G004 let msg_pos = usize::from(matches!(logging_call_type, LoggingCallType::LogCall)); if let Some(format_arg) = call.arguments.find_argument_value("msg", msg_pos) { - check_msg(checker, format_arg); + check_msg(checker, format_arg, &call.arguments, msg_pos); } // G010 diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G004.py.snap b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G004.py.snap index 10a31007b8a1a..f02d019ad5a50 100644 --- a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G004.py.snap +++ b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G004.py.snap @@ -9,6 +9,7 @@ G004 Logging statement uses f-string | ^^^^^^^^^^^^^^^ 5 | logging.log(logging.INFO, f"Hello {name}") | +help: Convert to lazy `%` formatting G004 Logging statement uses f-string --> G004.py:5:27 @@ -20,6 +21,7 @@ G004 Logging statement uses f-string 6 | 7 | _LOGGER = logging.getLogger() | +help: Convert to lazy `%` formatting G004 Logging statement uses f-string --> G004.py:8:14 @@ -30,6 +32,7 @@ G004 Logging statement uses f-string 9 | 10 | logging.getLogger().info(f"{name}") | +help: Convert to lazy `%` formatting G004 Logging statement uses f-string --> G004.py:10:26 @@ -41,6 +44,7 @@ G004 Logging statement uses f-string 11 | 12 | from logging import info | +help: Convert to lazy `%` formatting G004 Logging statement uses f-string --> G004.py:14:6 @@ -51,6 +55,7 @@ G004 Logging statement uses f-string | ^^^^^^^^^ 15 | info(f"{__name__}") | +help: Convert to lazy `%` formatting G004 Logging statement uses f-string --> G004.py:15:6 @@ -61,3 +66,156 @@ G004 Logging statement uses f-string 16 | 17 | # Don't trigger for t-strings | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:24:14 + | +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") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:30:13 + | +28 | x = 99 +29 | fmt = "08d" +30 | logger.info(f"{x:{'08d'}}") + | ^^^^^^^^^^^^^^ +31 | logger.info(f"{x:>10} {x:{fmt}}") + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:31:13 + | +29 | fmt = "08d" +30 | logger.info(f"{x:{'08d'}}") +31 | logger.info(f"{x:>10} {x:{fmt}}") + | ^^^^^^^^^^^^^^^^^^^^ +32 | +33 | logging.info(f"") + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:33:14 + | +31 | logger.info(f"{x:>10} {x:{fmt}}") +32 | +33 | logging.info(f"") + | ^^^ +34 | logging.info(f"This message doesn't have any variables.") + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:34:14 + | +33 | logging.info(f"") +34 | logging.info(f"This message doesn't have any variables.") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +35 | +36 | obj = {"key": "value"} + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:37:14 + | +36 | obj = {"key": "value"} +37 | logging.info(f"Object: {obj!r}") + | ^^^^^^^^^^^^^^^^^^ +38 | +39 | items_count = 3 + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:40:17 + | +39 | items_count = 3 +40 | logging.warning(f"Items: {items_count:d}") + | ^^^^^^^^^^^^^^^^^^^^^^^^^ +41 | +42 | data = {"status": "active"} + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:43:14 + | +42 | data = {"status": "active"} +43 | logging.info(f"Processing {len(data)} items") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +44 | logging.info(f"Status: {data.get('status', 'unknown').upper()}") + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:44:14 + | +42 | data = {"status": "active"} +43 | logging.info(f"Processing {len(data)} items") +44 | logging.info(f"Status: {data.get('status', 'unknown').upper()}") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:48:14 + | +47 | result = 123 +48 | logging.info(f"Calculated result: {result + 100}") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +49 | +50 | temperature = 123 + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:51:14 + | +50 | temperature = 123 +51 | logging.info(f"Temperature: {temperature:.1f}°C") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +52 | +53 | class FilePath: + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:57:14 + | +55 | self.name = name +56 | +57 | logging.info(f"No changes made to {file_path.name}.") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +58 | +59 | user = "tron" + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:61:15 + | +59 | user = "tron" +60 | balance = 123.45 +61 | logging.error(f"Error {404}: User {user} has insufficient balance ${balance:.2f}") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +62 | +63 | import logging + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:66:15 + | +65 | x = 1 +66 | logging.error(f"{x} -> %s", x) + | ^^^^^^^^^^^^ + | +help: Convert to lazy `%` formatting diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G004_arg_order.py.snap b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G004_arg_order.py.snap new file mode 100644 index 0000000000000..cb976e768a6b7 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__G004_arg_order.py.snap @@ -0,0 +1,23 @@ +--- +source: crates/ruff_linter/src/rules/flake8_logging_format/mod.rs +--- +G004 Logging statement uses f-string + --> G004_arg_order.py:9:14 + | + 7 | X = 1 + 8 | Y = 2 + 9 | logger.error(f"{X} -> %s", Y) + | ^^^^^^^^^^^^ +10 | logger.error(f"{Y} -> %s", X) + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004_arg_order.py:10:14 + | + 8 | Y = 2 + 9 | logger.error(f"{X} -> %s", Y) +10 | logger.error(f"{Y} -> %s", X) + | ^^^^^^^^^^^^ + | +help: Convert to lazy `%` formatting diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/violations.rs b/crates/ruff_linter/src/rules/flake8_logging_format/violations.rs index 51cc9f436ff6e..21e22bf2f71c0 100644 --- a/crates/ruff_linter/src/rules/flake8_logging_format/violations.rs +++ b/crates/ruff_linter/src/rules/flake8_logging_format/violations.rs @@ -327,10 +327,16 @@ impl Violation for LoggingStringConcat { pub(crate) struct LoggingFString; impl Violation for LoggingFString { + const FIX_AVAILABILITY: crate::FixAvailability = crate::FixAvailability::Sometimes; + #[derive_message_formats] fn message(&self) -> String { "Logging statement uses f-string".to_string() } + + fn fix_title(&self) -> Option { + Some("Convert to lazy `%` formatting".to_string()) + } } /// ## What it does From da42515d81f43fe8f754115eb0530a12c88b8caf Mon Sep 17 00:00:00 2001 From: Hamir Mahal Date: Sun, 24 Aug 2025 13:19:25 -0700 Subject: [PATCH 2/6] style: space before `fn logging_f_string` Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com> --- .../src/rules/flake8_logging_format/rules/logging_call.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs b/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs index 225887dc48fac..eac91ac2d0449 100644 --- a/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs +++ b/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs @@ -12,6 +12,7 @@ use crate::rules::flake8_logging_format::violations::{ LoggingRedundantExcInfo, LoggingStringConcat, LoggingStringFormat, LoggingWarn, }; use crate::{Edit, Fix}; + fn logging_f_string( checker: &Checker, msg: &Expr, From 7a974774bf151711522ef3427be395c3dc948f92 Mon Sep 17 00:00:00 2001 From: Hamir Mahal Date: Sun, 24 Aug 2025 13:19:45 -0700 Subject: [PATCH 3/6] refactor: reuse `args` vector instead of cloning --- .../src/rules/flake8_logging_format/rules/logging_call.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs b/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs index eac91ac2d0449..62bdfb6a95134 100644 --- a/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs +++ b/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs @@ -89,17 +89,13 @@ fn logging_f_string( }) .collect(); - // Combine collected interpolation names with existing trailing positional names in - // that order so that placeholders map to arguments correctly. Do not deduplicate; - // duplicates are valid when the same name appears multiple times. - let mut result_args = args.clone(); - result_args.extend(existing_names); + args.extend(existing_names); let replacement = format!( "{q}{format_string}{q}, {args}", q = quote_str, format_string = format_string, - args = result_args.join(", ") + args = args.join(", ") ); let fix = Fix::safe_edit(Edit::range_replacement(replacement, msg.range())); From c07120b5e497bfcbfc495d48a0b27dffc9b47b27 Mon Sep 17 00:00:00 2001 From: Hamir Mahal Date: Sun, 24 Aug 2025 13:20:05 -0700 Subject: [PATCH 4/6] refactor: remove unnecessary `.to_string()` call --- .../src/rules/flake8_logging_format/rules/logging_call.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs b/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs index 62bdfb6a95134..f954b03394245 100644 --- a/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs +++ b/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs @@ -38,8 +38,7 @@ fn logging_f_string( .f_strings() .next() .map(|f| f.flags.quote_str()) - .unwrap_or("\"") - .to_string(); + .unwrap_or("\""); for f in f_string.value.f_strings() { for element in &f.elements { From 7729095e1238cf38d25eeb7cb85c34b2728165ad Mon Sep 17 00:00:00 2001 From: Hamir Mahal Date: Sun, 24 Aug 2025 13:20:17 -0700 Subject: [PATCH 5/6] refactor: replace `Vec` with `Vec<&str>` --- .../src/rules/flake8_logging_format/rules/logging_call.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs b/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs index f954b03394245..f159910cd5550 100644 --- a/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs +++ b/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs @@ -29,7 +29,7 @@ fn logging_f_string( } let mut format_string = String::new(); - let mut args = Vec::new(); + let mut args: Vec<&str> = Vec::new(); // Try to reuse the first part's quote style when building the replacement. // Default to double quotes if we can't determine it. @@ -64,7 +64,7 @@ fn logging_f_string( match interpolated.expression.as_ref() { Expr::Name(name) => { format_string.push_str("%s"); - args.push(name.id.to_string()); + args.push(name.id.as_str()); } _ => return, } @@ -78,12 +78,12 @@ fn logging_f_string( } // Determine names of existing trailing positional args (after the `msg` argument). - let existing_names: Vec = arguments + let existing_names: Vec<&str> = arguments .args .iter() .skip(msg_pos + 1) .filter_map(|arg| match arg { - Expr::Name(name) => Some(name.id.to_string()), + Expr::Name(name) => Some(name.id.as_str()), _ => None, }) .collect(); From 7e9ed77a1ceca2eda8330c66cf3cc21149be03b6 Mon Sep 17 00:00:00 2001 From: Hamir Mahal Date: Sun, 24 Aug 2025 15:09:00 -0700 Subject: [PATCH 6/6] address remaining pull request feedback --- .../src/rules/flake8_logging_format/mod.rs | 48 +-- .../rules/logging_call.rs | 19 +- ..._format__tests__preview__G004_G004.py.snap | 291 ++++++++++++++++++ ...ests__preview__G004_G004_arg_order.py.snap | 23 ++ 4 files changed, 335 insertions(+), 46 deletions(-) create mode 100644 crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__preview__G004_G004.py.snap create mode 100644 crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__preview__G004_G004_arg_order.py.snap diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/mod.rs b/crates/ruff_linter/src/rules/flake8_logging_format/mod.rs index 940c0df57c48e..cea8ce78e803a 100644 --- a/crates/ruff_linter/src/rules/flake8_logging_format/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_logging_format/mod.rs @@ -22,6 +22,7 @@ mod tests { #[test_case(Path::new("G002.py"))] #[test_case(Path::new("G003.py"))] #[test_case(Path::new("G004.py"))] + #[test_case(Path::new("G004_arg_order.py"))] #[test_case(Path::new("G010.py"))] #[test_case(Path::new("G101_1.py"))] #[test_case(Path::new("G101_2.py"))] @@ -49,42 +50,23 @@ mod tests { Ok(()) } - #[test] - fn g004_fix_gated_by_preview_flag() -> Result<()> { - use crate::settings::LinterSettings; - - let diagnostics = test_path( - Path::new("flake8_logging_format") - .join(Path::new("G004_arg_order.py")) - .as_path(), - &LinterSettings { - preview: crate::settings::types::PreviewMode::Disabled, - ..LinterSettings::for_rules(vec![Rule::LoggingFString]) - }, - )?; - - // When preview is disabled, diagnostics should be emitted but no fix should be attached. - assert!(diagnostics.iter().all(|d| d.fix().is_none())); - Ok(()) - } - - #[test] - fn g004_preserves_argument_order_in_fix() -> Result<()> { - use crate::settings::LinterSettings; - - // Use the fixture that contains a two-argument example to ensure ordering is preserved. + #[test_case(Rule::LoggingFString, Path::new("G004.py"))] + #[test_case(Rule::LoggingFString, Path::new("G004_arg_order.py"))] + fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> { + let snapshot = format!( + "preview__{}_{}", + rule_code.noqa_code(), + path.to_string_lossy() + ); let diagnostics = test_path( - Path::new("flake8_logging_format") - .join(Path::new("G004_arg_order.py")) - .as_path(), - &LinterSettings { - preview: crate::settings::types::PreviewMode::Enabled, - ..LinterSettings::for_rules(vec![Rule::LoggingFString]) + Path::new("flake8_logging_format").join(path).as_path(), + &settings::LinterSettings { + logger_objects: vec!["logging_setup.logger".to_string()], + preview: settings::types::PreviewMode::Enabled, + ..settings::LinterSettings::for_rule(rule_code) }, )?; - - // Assert by snapshot (disabled/enabled snapshots stored in snapshots/). - assert_diagnostics!("G004_arg_order.py".to_string(), diagnostics); + assert_diagnostics!(snapshot, diagnostics); Ok(()) } } diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs b/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs index f159910cd5550..e87cf3a38db3d 100644 --- a/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs +++ b/crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs @@ -28,6 +28,12 @@ fn logging_f_string( return; } + // If there are existing positional arguments after the message, bail out. + // This could indicate a mistake or complex usage we shouldn't try to fix. + if arguments.args.len() > msg_pos + 1 { + return; + } + let mut format_string = String::new(); let mut args: Vec<&str> = Vec::new(); @@ -77,19 +83,6 @@ fn logging_f_string( return; } - // Determine names of existing trailing positional args (after the `msg` argument). - let existing_names: Vec<&str> = arguments - .args - .iter() - .skip(msg_pos + 1) - .filter_map(|arg| match arg { - Expr::Name(name) => Some(name.id.as_str()), - _ => None, - }) - .collect(); - - args.extend(existing_names); - let replacement = format!( "{q}{format_string}{q}, {args}", q = quote_str, diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__preview__G004_G004.py.snap b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__preview__G004_G004.py.snap new file mode 100644 index 0000000000000..b74a9e2306b74 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__preview__G004_G004.py.snap @@ -0,0 +1,291 @@ +--- +source: crates/ruff_linter/src/rules/flake8_logging_format/mod.rs +--- +G004 [*] Logging statement uses f-string + --> G004.py:4:14 + | +3 | name = "world" +4 | logging.info(f"Hello {name}") + | ^^^^^^^^^^^^^^^ +5 | logging.log(logging.INFO, f"Hello {name}") + | +help: Convert to lazy `%` formatting + +ℹ Safe fix +1 1 | import logging +2 2 | +3 3 | name = "world" +4 |-logging.info(f"Hello {name}") + 4 |+logging.info("Hello %s", name) +5 5 | logging.log(logging.INFO, f"Hello {name}") +6 6 | +7 7 | _LOGGER = logging.getLogger() + +G004 [*] Logging statement uses f-string + --> G004.py:5:27 + | +3 | name = "world" +4 | logging.info(f"Hello {name}") +5 | logging.log(logging.INFO, f"Hello {name}") + | ^^^^^^^^^^^^^^^ +6 | +7 | _LOGGER = logging.getLogger() + | +help: Convert to lazy `%` formatting + +ℹ Safe fix +2 2 | +3 3 | name = "world" +4 4 | logging.info(f"Hello {name}") +5 |-logging.log(logging.INFO, f"Hello {name}") + 5 |+logging.log(logging.INFO, "Hello %s", name) +6 6 | +7 7 | _LOGGER = logging.getLogger() +8 8 | _LOGGER.info(f"{__name__}") + +G004 [*] Logging statement uses f-string + --> G004.py:8:14 + | + 7 | _LOGGER = logging.getLogger() + 8 | _LOGGER.info(f"{__name__}") + | ^^^^^^^^^^^^^ + 9 | +10 | logging.getLogger().info(f"{name}") + | +help: Convert to lazy `%` formatting + +ℹ Safe fix +5 5 | logging.log(logging.INFO, f"Hello {name}") +6 6 | +7 7 | _LOGGER = logging.getLogger() +8 |-_LOGGER.info(f"{__name__}") + 8 |+_LOGGER.info("%s", __name__) +9 9 | +10 10 | logging.getLogger().info(f"{name}") +11 11 | + +G004 [*] Logging statement uses f-string + --> G004.py:10:26 + | + 8 | _LOGGER.info(f"{__name__}") + 9 | +10 | logging.getLogger().info(f"{name}") + | ^^^^^^^^^ +11 | +12 | from logging import info + | +help: Convert to lazy `%` formatting + +ℹ Safe fix +7 7 | _LOGGER = logging.getLogger() +8 8 | _LOGGER.info(f"{__name__}") +9 9 | +10 |-logging.getLogger().info(f"{name}") + 10 |+logging.getLogger().info("%s", name) +11 11 | +12 12 | from logging import info +13 13 | + +G004 [*] Logging statement uses f-string + --> G004.py:14:6 + | +12 | from logging import info +13 | +14 | info(f"{name}") + | ^^^^^^^^^ +15 | info(f"{__name__}") + | +help: Convert to lazy `%` formatting + +ℹ Safe fix +11 11 | +12 12 | from logging import info +13 13 | +14 |-info(f"{name}") + 14 |+info("%s", name) +15 15 | info(f"{__name__}") +16 16 | +17 17 | # Don't trigger for t-strings + +G004 [*] Logging statement uses f-string + --> G004.py:15:6 + | +14 | info(f"{name}") +15 | info(f"{__name__}") + | ^^^^^^^^^^^^^ +16 | +17 | # Don't trigger for t-strings + | +help: Convert to lazy `%` formatting + +ℹ Safe fix +12 12 | from logging import info +13 13 | +14 14 | info(f"{name}") +15 |-info(f"{__name__}") + 15 |+info("%s", __name__) +16 16 | +17 17 | # Don't trigger for t-strings +18 18 | info(t"{name}") + +G004 [*] Logging statement uses f-string + --> G004.py:24:14 + | +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") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: Convert to lazy `%` formatting + +ℹ Safe fix +21 21 | count = 5 +22 22 | total = 9 +23 23 | directory_path = "/home/hamir/ruff/crates/ruff_linter/resources/test/" +24 |-logging.info(f"{count} out of {total} files in {directory_path} checked") + 24 |+logging.info("%s out of %s files in %s checked", count, total, directory_path) +25 25 | +26 26 | +27 27 | + +G004 Logging statement uses f-string + --> G004.py:30:13 + | +28 | x = 99 +29 | fmt = "08d" +30 | logger.info(f"{x:{'08d'}}") + | ^^^^^^^^^^^^^^ +31 | logger.info(f"{x:>10} {x:{fmt}}") + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:31:13 + | +29 | fmt = "08d" +30 | logger.info(f"{x:{'08d'}}") +31 | logger.info(f"{x:>10} {x:{fmt}}") + | ^^^^^^^^^^^^^^^^^^^^ +32 | +33 | logging.info(f"") + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:33:14 + | +31 | logger.info(f"{x:>10} {x:{fmt}}") +32 | +33 | logging.info(f"") + | ^^^ +34 | logging.info(f"This message doesn't have any variables.") + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:34:14 + | +33 | logging.info(f"") +34 | logging.info(f"This message doesn't have any variables.") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +35 | +36 | obj = {"key": "value"} + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:37:14 + | +36 | obj = {"key": "value"} +37 | logging.info(f"Object: {obj!r}") + | ^^^^^^^^^^^^^^^^^^ +38 | +39 | items_count = 3 + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:40:17 + | +39 | items_count = 3 +40 | logging.warning(f"Items: {items_count:d}") + | ^^^^^^^^^^^^^^^^^^^^^^^^^ +41 | +42 | data = {"status": "active"} + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:43:14 + | +42 | data = {"status": "active"} +43 | logging.info(f"Processing {len(data)} items") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +44 | logging.info(f"Status: {data.get('status', 'unknown').upper()}") + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:44:14 + | +42 | data = {"status": "active"} +43 | logging.info(f"Processing {len(data)} items") +44 | logging.info(f"Status: {data.get('status', 'unknown').upper()}") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:48:14 + | +47 | result = 123 +48 | logging.info(f"Calculated result: {result + 100}") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +49 | +50 | temperature = 123 + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:51:14 + | +50 | temperature = 123 +51 | logging.info(f"Temperature: {temperature:.1f}°C") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +52 | +53 | class FilePath: + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:57:14 + | +55 | self.name = name +56 | +57 | logging.info(f"No changes made to {file_path.name}.") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +58 | +59 | user = "tron" + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:61:15 + | +59 | user = "tron" +60 | balance = 123.45 +61 | logging.error(f"Error {404}: User {user} has insufficient balance ${balance:.2f}") + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +62 | +63 | import logging + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004.py:66:15 + | +65 | x = 1 +66 | logging.error(f"{x} -> %s", x) + | ^^^^^^^^^^^^ + | +help: Convert to lazy `%` formatting diff --git a/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__preview__G004_G004_arg_order.py.snap b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__preview__G004_G004_arg_order.py.snap new file mode 100644 index 0000000000000..cb976e768a6b7 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_logging_format/snapshots/ruff_linter__rules__flake8_logging_format__tests__preview__G004_G004_arg_order.py.snap @@ -0,0 +1,23 @@ +--- +source: crates/ruff_linter/src/rules/flake8_logging_format/mod.rs +--- +G004 Logging statement uses f-string + --> G004_arg_order.py:9:14 + | + 7 | X = 1 + 8 | Y = 2 + 9 | logger.error(f"{X} -> %s", Y) + | ^^^^^^^^^^^^ +10 | logger.error(f"{Y} -> %s", X) + | +help: Convert to lazy `%` formatting + +G004 Logging statement uses f-string + --> G004_arg_order.py:10:14 + | + 8 | Y = 2 + 9 | logger.error(f"{X} -> %s", Y) +10 | logger.error(f"{Y} -> %s", X) + | ^^^^^^^^^^^^ + | +help: Convert to lazy `%` formatting