Fix logging rules with whitespace around dot (#6022)

## Summary Attempting to fix, e.g., `logging . warn("Hello World!")` was causing a syntax error.
astral-sh · Jul 24, 2023 · 33196f1 · 33196f1
1 parent 0d94337
commit 33196f1
Show file tree

Hide file tree

Showing 4 changed files with 88 additions and 66 deletions.
diff --git a/crates/ruff/resources/test/fixtures/flake8_logging_format/G010.py b/crates/ruff/resources/test/fixtures/flake8_logging_format/G010.py
@@ -6,3 +6,5 @@
 logging.warn("Hello World!")
 log.warn("Hello world!")  # This shouldn't be considered as a logger candidate
 logger.warn("Hello world!")
+
+logging . warn("Hello World!")
diff --git a/crates/ruff/src/rules/flake8_logging_format/rules/logging_call.rs b/crates/ruff/src/rules/flake8_logging_format/rules/logging_call.rs
@@ -1,4 +1,3 @@
-use ruff_text_size::{TextRange, TextSize};
 use rustpython_parser::ast::{self, Constant, Expr, Keyword, Operator, Ranged};
 
 use ruff_diagnostics::{Diagnostic, Edit, Fix};
@@ -133,7 +132,7 @@ fn check_log_record_attr_clash(checker: &mut Checker, extra: &Keyword) {
     }
 }
 
-#[derive(Copy, Clone)]
+#[derive(Debug, Copy, Clone)]
 enum LoggingCallType {
     /// Logging call with a level method, e.g., `logging.info`.
     LevelCall(LoggingLevel),
@@ -158,73 +157,73 @@ pub(crate) fn logging_call(
     args: &[Expr],
     keywords: &[Keyword],
 ) {
+    let Expr::Attribute(ast::ExprAttribute { value: _, attr, .. }) = func else {
+        return;
+    };
+
+    let Some(logging_call_type) = LoggingCallType::from_attribute(attr.as_str()) else {
+        return;
+    };
+
     if !logging::is_logger_candidate(func, checker.semantic(), &checker.settings.logger_objects) {
         return;
     }
 
-    if let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = func {
-        if let Some(logging_call_type) = LoggingCallType::from_attribute(attr.as_str()) {
-            let call_args = CallArguments::new(args, keywords);
-            let level_call_range = TextRange::new(value.end() + TextSize::from(1), func.end());
-
-            // G001 - G004
-            let msg_pos = usize::from(matches!(logging_call_type, LoggingCallType::LogCall));
-            if let Some(format_arg) = call_args.argument("msg", msg_pos) {
-                check_msg(checker, format_arg);
-            }
+    // G001 - G004
+    let msg_pos = usize::from(matches!(logging_call_type, LoggingCallType::LogCall));
+    if let Some(format_arg) = CallArguments::new(args, keywords).argument("msg", msg_pos) {
+        check_msg(checker, format_arg);
+    }
 
-            // G010
-            if checker.enabled(Rule::LoggingWarn)
-                && matches!(
-                    logging_call_type,
-                    LoggingCallType::LevelCall(LoggingLevel::Warn)
-                )
-            {
-                let mut diagnostic = Diagnostic::new(LoggingWarn, level_call_range);
-                if checker.patch(diagnostic.kind.rule()) {
-                    diagnostic.set_fix(Fix::automatic(Edit::range_replacement(
-                        "warning".to_string(),
-                        level_call_range,
-                    )));
-                }
-                checker.diagnostics.push(diagnostic);
+    // G010
+    if checker.enabled(Rule::LoggingWarn) {
+        if matches!(
+            logging_call_type,
+            LoggingCallType::LevelCall(LoggingLevel::Warn)
+        ) {
+            let mut diagnostic = Diagnostic::new(LoggingWarn, attr.range());
+            if checker.patch(diagnostic.kind.rule()) {
+                diagnostic.set_fix(Fix::automatic(Edit::range_replacement(
+                    "warning".to_string(),
+                    attr.range(),
+                )));
             }
+            checker.diagnostics.push(diagnostic);
+        }
+    }
 
-            // G101
-            if checker.enabled(Rule::LoggingExtraAttrClash) {
-                if let Some(extra) = find_keyword(keywords, "extra") {
-                    check_log_record_attr_clash(checker, extra);
-                }
-            }
+    // G101
+    if checker.enabled(Rule::LoggingExtraAttrClash) {
+        if let Some(extra) = find_keyword(keywords, "extra") {
+            check_log_record_attr_clash(checker, extra);
+        }
+    }
 
-            // G201, G202
-            if checker.any_enabled(&[Rule::LoggingExcInfo, Rule::LoggingRedundantExcInfo]) {
-                if !checker.semantic().in_exception_handler() {
-                    return;
+    // G201, G202
+    if checker.any_enabled(&[Rule::LoggingExcInfo, Rule::LoggingRedundantExcInfo]) {
+        if !checker.semantic().in_exception_handler() {
+            return;
+        }
+        let Some(exc_info) = logging::exc_info(keywords, checker.semantic()) else {
+            return;
+        };
+        if let LoggingCallType::LevelCall(logging_level) = logging_call_type {
+            match logging_level {
+                LoggingLevel::Error => {
+                    if checker.enabled(Rule::LoggingExcInfo) {
+                        checker
+                            .diagnostics
+                            .push(Diagnostic::new(LoggingExcInfo, attr.range()));
+                    }
                 }
-                let Some(exc_info) = logging::exc_info(keywords, checker.semantic()) else {
-                    return;
-                };
-                if let LoggingCallType::LevelCall(logging_level) = logging_call_type {
-                    match logging_level {
-                        LoggingLevel::Error => {
-                            if checker.enabled(Rule::LoggingExcInfo) {
-                                checker
-                                    .diagnostics
-                                    .push(Diagnostic::new(LoggingExcInfo, level_call_range));
-                            }
-                        }
-                        LoggingLevel::Exception => {
-                            if checker.enabled(Rule::LoggingRedundantExcInfo) {
-                                checker.diagnostics.push(Diagnostic::new(
-                                    LoggingRedundantExcInfo,
-                                    exc_info.range(),
-                                ));
-                            }
-                        }
-                        _ => {}
+                LoggingLevel::Exception => {
+                    if checker.enabled(Rule::LoggingRedundantExcInfo) {
+                        checker
+                            .diagnostics
+                            .push(Diagnostic::new(LoggingRedundantExcInfo, exc_info.range()));
                     }
                 }
+                _ => {}
             }
         }
     }

diff --git a/...s/flake8_logging_format/snapshots/ruff__rules__flake8_logging_format__tests__G010.py.snap b/...s/flake8_logging_format/snapshots/ruff__rules__flake8_logging_format__tests__G010.py.snap
@@ -20,21 +20,42 @@ G010.py:6:9: G010 [*] Logging statement uses `warn` instead of `warning`
   6 |+logging.warning("Hello World!")
 7 7 | log.warn("Hello world!")  # This shouldn't be considered as a logger candidate
 8 8 | logger.warn("Hello world!")
+9 9 | 
 
 G010.py:8:8: G010 [*] Logging statement uses `warn` instead of `warning`
-  |
-6 | logging.warn("Hello World!")
-7 | log.warn("Hello world!")  # This shouldn't be considered as a logger candidate
-8 | logger.warn("Hello world!")
-  |        ^^^^ G010
-  |
-  = help: Convert to `warn`
+   |
+ 6 | logging.warn("Hello World!")
+ 7 | log.warn("Hello world!")  # This shouldn't be considered as a logger candidate
+ 8 | logger.warn("Hello world!")
+   |        ^^^^ G010
+ 9 | 
+10 | logging . warn("Hello World!")
+   |
+   = help: Convert to `warn`
 
 ℹ Fix
 5 5 | 
 6 6 | logging.warn("Hello World!")
 7 7 | log.warn("Hello world!")  # This shouldn't be considered as a logger candidate
 8   |-logger.warn("Hello world!")
   8 |+logger.warning("Hello world!")
+9 9 | 
+10 10 | logging . warn("Hello World!")
+
+G010.py:10:11: G010 [*] Logging statement uses `warn` instead of `warning`
+   |
+ 8 | logger.warn("Hello world!")
+ 9 | 
+10 | logging . warn("Hello World!")
+   |           ^^^^ G010
+   |
+   = help: Convert to `warn`
+
+ℹ Fix
+7  7  | log.warn("Hello world!")  # This shouldn't be considered as a logger candidate
+8  8  | logger.warn("Hello world!")
+9  9  | 
+10    |-logging . warn("Hello World!")
+   10 |+logging . warning("Hello World!")
 
 
diff --git a/crates/ruff_python_stdlib/src/logging.rs b/crates/ruff_python_stdlib/src/logging.rs
@@ -1,4 +1,4 @@
-#[derive(Copy, Clone)]
+#[derive(Debug, Copy, Clone)]
 pub enum LoggingLevel {
     Debug,
     Critical,