Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flake8-logging] Implement LOG002: invalid-get-logger-argument #7399

Merged
merged 2 commits into from
Sep 16, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[flake8-logging] Implement LOG002: invalid-get-logger-argument
  • Loading branch information
dhruvmanila committed Sep 15, 2023
commit 2b0538b4b323a9c6b5663acb49357a4f18328a7a
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,83 @@
use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix};
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 AlwaysAutofixableViolation for InvalidGetLoggerArgument {
#[derive_message_formats]
fn message(&self) -> String {
format!("Use `__name__` with `logging.getLogger()`")
}

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

/// LOG002
pub(crate) fn invalid_get_logger_argument(checker: &mut Checker, call: &ast::ExprCall) {
if checker
.semantic()
.resolve_call_path(call.func.as_ref())
.is_some_and(|call_path| matches!(call_path.as_slice(), ["logging", "getLogger"]))
{
let Some(Expr::Name(expr @ ast::ExprName { id, .. })) =
call.arguments.find_argument("name", 0)
else {
return;
};

if !matches!(id.as_ref(), "__file__" | "__cached__") {
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved
return;
}
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved

let mut diagnostic = Diagnostic::new(InvalidGetLoggerArgument, expr.range());
if checker.patch(diagnostic.kind.rule()) {
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_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.

Loading