Skip to content

Commit

Permalink
[flake8-logging] Implement LOG002: invalid-get-logger-argument (#…
Browse files Browse the repository at this point in the history
…7399)

## Summary

This PR implements a new rule for `flake8-logging` plugin that checks
for
`logging.getLogger` calls with either `__file__` or `__cached__` as the
first
argument and generates a suggested fix to use `__name__` instead.

Refer: #7248

## Test Plan

Add test cases and `cargo test`
  • Loading branch information
dhruvmanila authored Sep 16, 2023
1 parent c907317 commit 0d1fb82
Show file tree
Hide file tree
Showing 10 changed files with 208 additions and 0 deletions.
24 changes: 24 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_logging/LOG002.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import logging
from logging import getLogger

# Ok
logging.getLogger(__name__)
logging.getLogger(name=__name__)
logging.getLogger("custom")
logging.getLogger(name="custom")

# LOG002
getLogger(__file__)
logging.getLogger(name=__file__)

logging.getLogger(__cached__)
getLogger(name=__cached__)


# Override `logging.getLogger`
class logging:
def getLogger(self):
pass


logging.getLogger(__file__)
3 changes: 3 additions & 0 deletions crates/ruff/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -895,6 +895,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::DirectLoggerInstantiation) {
flake8_logging::rules::direct_logger_instantiation(checker, call);
}
if checker.enabled(Rule::InvalidGetLoggerArgument) {
flake8_logging::rules::invalid_get_logger_argument(checker, call);
}
}
Expr::Dict(ast::ExprDict {
keys,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -922,6 +922,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {

// flake8-logging
(Flake8Logging, "001") => (RuleGroup::Preview, rules::flake8_logging::rules::DirectLoggerInstantiation),
(Flake8Logging, "002") => (RuleGroup::Preview, rules::flake8_logging::rules::InvalidGetLoggerArgument),
(Flake8Logging, "009") => (RuleGroup::Preview, rules::flake8_logging::rules::UndocumentedWarn),

_ => return None,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/rules/flake8_logging/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ mod tests {
use crate::test::test_path;

#[test_case(Rule::DirectLoggerInstantiation, Path::new("LOG001.py"))]
#[test_case(Rule::InvalidGetLoggerArgument, Path::new("LOG002.py"))]
#[test_case(Rule::UndocumentedWarn, Path::new("LOG009.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Expr};
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
use crate::registry::AsRule;

/// ## What it does
/// Checks for any usage of `__cached__` and `__file__` as an argument to
/// `logging.getLogger()`.
///
/// ## Why is this bad?
/// The [logging documentation] recommends this pattern:
///
/// ```python
/// logging.getLogger(__name__)
/// ```
///
/// Here, `__name__` is the fully qualified module name, such as `foo.bar`,
/// which is the intended format for logger names.
///
/// This rule detects probably-mistaken usage of similar module-level dunder constants:
///
/// * `__cached__` - the pathname of the module's compiled version, such as `foo/__pycache__/bar.cpython-311.pyc`.
/// * `__file__` - the pathname of the module, such as `foo/bar.py`.
///
/// ## Example
/// ```python
/// import logging
///
/// logger = logging.getLogger(__file__)
/// ```
///
/// Use instead:
/// ```python
/// import logging
///
/// logger = logging.getLogger(__name__)
/// ```
///
/// [logging documentation]: https://docs.python.org/3/library/logging.html#logger-objects
#[violation]
pub struct InvalidGetLoggerArgument;

impl Violation for InvalidGetLoggerArgument {
const AUTOFIX: AutofixKind = AutofixKind::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
format!("Use `__name__` with `logging.getLogger()`")
}

fn autofix_title(&self) -> Option<String> {
Some(format!("Replace with `__name__`"))
}
}

/// LOG002
pub(crate) fn invalid_get_logger_argument(checker: &mut Checker, call: &ast::ExprCall) {
let Some(Expr::Name(expr @ ast::ExprName { id, .. })) = call.arguments.find_argument("name", 0)
else {
return;
};

if !matches!(id.as_ref(), "__file__" | "__cached__") {
return;
}

if !checker.semantic().is_builtin(id) {
return;
}

if !checker
.semantic()
.resolve_call_path(call.func.as_ref())
.is_some_and(|call_path| matches!(call_path.as_slice(), ["logging", "getLogger"]))
{
return;
}

let mut diagnostic = Diagnostic::new(InvalidGetLoggerArgument, expr.range());
if checker.patch(diagnostic.kind.rule()) {
if checker.semantic().is_builtin("__name__") {
diagnostic.set_fix(Fix::suggested(Edit::range_replacement(
"__name__".to_string(),
expr.range(),
)));
}
}
checker.diagnostics.push(diagnostic);
}
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/flake8_logging/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
pub(crate) use direct_logger_instantiation::*;
pub(crate) use invalid_get_logger_argument::*;
pub(crate) use undocumented_warn::*;

mod direct_logger_instantiation;
mod invalid_get_logger_argument;
mod undocumented_warn;
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
---
source: crates/ruff/src/rules/flake8_logging/mod.rs
---
LOG002.py:11:11: LOG002 [*] Use `__name__` with `logging.getLogger()`
|
10 | # LOG002
11 | getLogger(__file__)
| ^^^^^^^^ LOG002
12 | logging.getLogger(name=__file__)
|
= help: Replace with `name`

Suggested fix
8 8 | logging.getLogger(name="custom")
9 9 |
10 10 | # LOG002
11 |-getLogger(__file__)
11 |+getLogger(__name__)
12 12 | logging.getLogger(name=__file__)
13 13 |
14 14 | logging.getLogger(__cached__)

LOG002.py:12:24: LOG002 [*] Use `__name__` with `logging.getLogger()`
|
10 | # LOG002
11 | getLogger(__file__)
12 | logging.getLogger(name=__file__)
| ^^^^^^^^ LOG002
13 |
14 | logging.getLogger(__cached__)
|
= help: Replace with `name`

Suggested fix
9 9 |
10 10 | # LOG002
11 11 | getLogger(__file__)
12 |-logging.getLogger(name=__file__)
12 |+logging.getLogger(name=__name__)
13 13 |
14 14 | logging.getLogger(__cached__)
15 15 | getLogger(name=__cached__)

LOG002.py:14:19: LOG002 [*] Use `__name__` with `logging.getLogger()`
|
12 | logging.getLogger(name=__file__)
13 |
14 | logging.getLogger(__cached__)
| ^^^^^^^^^^ LOG002
15 | getLogger(name=__cached__)
|
= help: Replace with `name`

Suggested fix
11 11 | getLogger(__file__)
12 12 | logging.getLogger(name=__file__)
13 13 |
14 |-logging.getLogger(__cached__)
14 |+logging.getLogger(__name__)
15 15 | getLogger(name=__cached__)
16 16 |
17 17 |

LOG002.py:15:16: LOG002 [*] Use `__name__` with `logging.getLogger()`
|
14 | logging.getLogger(__cached__)
15 | getLogger(name=__cached__)
| ^^^^^^^^^^ LOG002
|
= help: Replace with `name`

Suggested fix
12 12 | logging.getLogger(name=__file__)
13 13 |
14 14 | logging.getLogger(__cached__)
15 |-getLogger(name=__cached__)
15 |+getLogger(name=__name__)
16 16 |
17 17 |
18 18 | # Override `logging.getLogger`


1 change: 1 addition & 0 deletions crates/ruff_python_stdlib/src/builtins.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ pub const MAGIC_GLOBALS: &[&str] = &[
"WindowsError",
"__annotations__",
"__builtins__",
"__cached__",
"__file__",
];

Expand Down
1 change: 1 addition & 0 deletions crates/ruff_workspace/src/configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -859,6 +859,7 @@ mod tests {
Rule::TooManyPublicMethods,
Rule::TooManyPublicMethods,
Rule::UndocumentedWarn,
Rule::InvalidGetLoggerArgument,
];

#[allow(clippy::needless_pass_by_value)]
Expand Down
1 change: 1 addition & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 0d1fb82

Please sign in to comment.