Skip to content

Commit

Permalink
Allow specification of logging.Logger re-exports via `logger-object…
Browse files Browse the repository at this point in the history
…s` (#5750)

## Summary

This PR adds a `logger-objects` setting that allows users to mark
specific symbols a `logging.Logger` objects. Currently, if a `logger` is
imported, we only flagged it as a `logging.Logger` if it comes exactly
from the `logging` module or is `flask.current_app.logger`.

This PR allows users to mark specific loggers, like
`logging_setup.logger`, to ensure that they're covered by the
`flake8-logging-format` rules and others.

For example, if you have a module `logging_setup.py` with the following
contents:

```python
import logging

logger = logging.getLogger(__name__)
```

Adding `"logging_setup.logger"` to `logger-objects` will ensure that
`logging_setup.logger` is treated as a `logging.Logger` object when
imported from other modules (e.g., `from logging_setup import logger`).

Closes #5694.
  • Loading branch information
charliermarsh authored Jul 24, 2023
1 parent 727153c commit f9726af
Show file tree
Hide file tree
Showing 16 changed files with 200 additions and 107 deletions.
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import logging
from distutils import log

from logging_setup import logger

logging.warn("Hello World!")
log.warn("Hello world!") # This shouldn't be considered as a logger candidate
logger.warn("Hello world!")
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,11 @@ pub(crate) fn blind_except(
if body.iter().any(|stmt| {
if let Stmt::Expr(ast::StmtExpr { value, range: _ }) = stmt {
if let Expr::Call(ast::ExprCall { func, keywords, .. }) = value.as_ref() {
if logging::is_logger_candidate(func, checker.semantic()) {
if logging::is_logger_candidate(
func,
checker.semantic(),
&checker.settings.logger_objects,
) {
if let Some(attribute) = func.as_attribute_expr() {
let attr = attribute.attr.as_str();
if attr == "exception" {
Expand Down
23 changes: 13 additions & 10 deletions crates/ruff/src/rules/flake8_logging_format/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,19 @@ mod tests {
let snapshot = path.to_string_lossy().into_owned();
let diagnostics = test_path(
Path::new("flake8_logging_format").join(path).as_path(),
&settings::Settings::for_rules(vec![
Rule::LoggingStringFormat,
Rule::LoggingPercentFormat,
Rule::LoggingStringConcat,
Rule::LoggingFString,
Rule::LoggingWarn,
Rule::LoggingExtraAttrClash,
Rule::LoggingExcInfo,
Rule::LoggingRedundantExcInfo,
]),
&settings::Settings {
logger_objects: vec!["logging_setup.logger".to_string()],
..settings::Settings::for_rules(vec![
Rule::LoggingStringFormat,
Rule::LoggingPercentFormat,
Rule::LoggingStringConcat,
Rule::LoggingFString,
Rule::LoggingWarn,
Rule::LoggingExtraAttrClash,
Rule::LoggingExcInfo,
Rule::LoggingRedundantExcInfo,
])
},
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ pub(crate) fn logging_call(
args: &[Expr],
keywords: &[Keyword],
) {
if !logging::is_logger_candidate(func, checker.semantic()) {
if !logging::is_logger_candidate(func, checker.semantic(), &checker.settings.logger_objects) {
return;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,22 +1,40 @@
---
source: crates/ruff/src/rules/flake8_logging_format/mod.rs
---
G010.py:4:9: G010 [*] Logging statement uses `warn` instead of `warning`
G010.py:6:9: G010 [*] Logging statement uses `warn` instead of `warning`
|
2 | from distutils import log
3 |
4 | logging.warn("Hello World!")
4 | from logging_setup import logger
5 |
6 | logging.warn("Hello World!")
| ^^^^ G010
5 | log.warn("Hello world!") # This shouldn't be considered as a logger candidate
7 | log.warn("Hello world!") # This shouldn't be considered as a logger candidate
8 | logger.warn("Hello world!")
|
= help: Convert to `warn`

Fix
1 1 | import logging
2 2 | from distutils import log
3 3 |
4 |-logging.warn("Hello World!")
4 |+logging.warning("Hello World!")
5 5 | log.warn("Hello world!") # This shouldn't be considered as a logger candidate
4 4 | from logging_setup import logger
5 5 |
6 |-logging.warn("Hello World!")
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!")

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`

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!")


85 changes: 46 additions & 39 deletions crates/ruff/src/rules/pylint/rules/logging.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,48 +102,55 @@ pub(crate) fn logging_call(
return;
}

if !logging::is_logger_candidate(func, checker.semantic()) {
if !logging::is_logger_candidate(func, checker.semantic(), &checker.settings.logger_objects) {
return;
}

if let Expr::Attribute(ast::ExprAttribute { attr, .. }) = func {
if LoggingLevel::from_attribute(attr.as_str()).is_some() {
let call_args = SimpleCallArgs::new(args, keywords);
if let Some(Expr::Constant(ast::ExprConstant {
value: Constant::Str(value),
..
})) = call_args.argument("msg", 0)
{
if let Ok(summary) = CFormatSummary::try_from(value.as_str()) {
if summary.starred {
return;
}
if !summary.keywords.is_empty() {
return;
}

let message_args = call_args.num_args() - 1;

if checker.enabled(Rule::LoggingTooManyArgs) {
if summary.num_positional < message_args {
checker
.diagnostics
.push(Diagnostic::new(LoggingTooManyArgs, func.range()));
}
}

if checker.enabled(Rule::LoggingTooFewArgs) {
if message_args > 0
&& call_args.num_kwargs() == 0
&& summary.num_positional > message_args
{
checker
.diagnostics
.push(Diagnostic::new(LoggingTooFewArgs, func.range()));
}
}
}
}
let Expr::Attribute(ast::ExprAttribute { attr, .. }) = func else {
return;
};

if LoggingLevel::from_attribute(attr.as_str()).is_none() {
return;
}

let call_args = SimpleCallArgs::new(args, keywords);
let Some(Expr::Constant(ast::ExprConstant {
value: Constant::Str(value),
..
})) = call_args.argument("msg", 0)
else {
return;
};

let Ok(summary) = CFormatSummary::try_from(value.as_str()) else {
return;
};

if summary.starred {
return;
}

if !summary.keywords.is_empty() {
return;
}

let message_args = call_args.num_args() - 1;

if checker.enabled(Rule::LoggingTooManyArgs) {
if summary.num_positional < message_args {
checker
.diagnostics
.push(Diagnostic::new(LoggingTooManyArgs, func.range()));
}
}

if checker.enabled(Rule::LoggingTooFewArgs) {
if message_args > 0 && call_args.num_kwargs() == 0 && summary.num_positional > message_args
{
checker
.diagnostics
.push(Diagnostic::new(LoggingTooFewArgs, func.range()));
}
}
}
6 changes: 4 additions & 2 deletions crates/ruff/src/rules/tryceratops/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,15 @@ use ruff_python_semantic::SemanticModel;
/// Collect `logging`-like calls from an AST.
pub(super) struct LoggerCandidateVisitor<'a, 'b> {
semantic: &'a SemanticModel<'b>,
logger_objects: &'a [String],
pub(super) calls: Vec<&'b ast::ExprCall>,
}

impl<'a, 'b> LoggerCandidateVisitor<'a, 'b> {
pub(super) fn new(semantic: &'a SemanticModel<'b>) -> Self {
pub(super) fn new(semantic: &'a SemanticModel<'b>, logger_objects: &'a [String]) -> Self {
LoggerCandidateVisitor {
semantic,
logger_objects,
calls: Vec::new(),
}
}
Expand All @@ -23,7 +25,7 @@ impl<'a, 'b> LoggerCandidateVisitor<'a, 'b> {
impl<'a, 'b> Visitor<'b> for LoggerCandidateVisitor<'a, 'b> {
fn visit_expr(&mut self, expr: &'b Expr) {
if let Expr::Call(call) = expr {
if logging::is_logger_candidate(&call.func, self.semantic) {
if logging::is_logger_candidate(&call.func, self.semantic, self.logger_objects) {
self.calls.push(call);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ pub(crate) fn error_instead_of_exception(checker: &mut Checker, handlers: &[Exce
for handler in handlers {
let ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { body, .. }) = handler;
let calls = {
let mut visitor = LoggerCandidateVisitor::new(checker.semantic());
let mut visitor =
LoggerCandidateVisitor::new(checker.semantic(), &checker.settings.logger_objects);
visitor.visit_body(body);
visitor.calls
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,8 @@ pub(crate) fn verbose_log_message(checker: &mut Checker, handlers: &[ExceptHandl

// Find all calls to `logging.exception`.
let calls = {
let mut visitor = LoggerCandidateVisitor::new(checker.semantic());
let mut visitor =
LoggerCandidateVisitor::new(checker.semantic(), &checker.settings.logger_objects);
visitor.visit_body(body);
visitor.calls
};
Expand Down
5 changes: 4 additions & 1 deletion crates/ruff/src/settings/configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,14 @@ pub struct Configuration {
pub ignore_init_module_imports: Option<bool>,
pub include: Option<Vec<FilePattern>>,
pub line_length: Option<LineLength>,
pub tab_size: Option<TabSize>,
pub logger_objects: Option<Vec<String>>,
pub namespace_packages: Option<Vec<PathBuf>>,
pub required_version: Option<Version>,
pub respect_gitignore: Option<bool>,
pub show_fixes: Option<bool>,
pub show_source: Option<bool>,
pub src: Option<Vec<PathBuf>>,
pub tab_size: Option<TabSize>,
pub target_version: Option<PythonVersion>,
pub task_tags: Option<Vec<String>>,
pub typing_modules: Option<Vec<String>>,
Expand Down Expand Up @@ -223,6 +224,7 @@ impl Configuration {
.transpose()?,
target_version: options.target_version,
task_tags: options.task_tags,
logger_objects: options.logger_objects,
typing_modules: options.typing_modules,
// Plugins
flake8_annotations: options.flake8_annotations,
Expand Down Expand Up @@ -291,6 +293,7 @@ impl Configuration {
.ignore_init_module_imports
.or(config.ignore_init_module_imports),
line_length: self.line_length.or(config.line_length),
logger_objects: self.logger_objects.or(config.logger_objects),
tab_size: self.tab_size.or(config.tab_size),
namespace_packages: self.namespace_packages.or(config.namespace_packages),
per_file_ignores: self.per_file_ignores.or(config.per_file_ignores),
Expand Down
5 changes: 3 additions & 2 deletions crates/ruff/src/settings/defaults.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,12 +84,13 @@ impl Default for Settings {
ignore_init_module_imports: false,
include: FilePatternSet::try_from_vec(INCLUDE.clone()).unwrap(),
line_length: LineLength::default(),
tab_size: TabSize::default(),
logger_objects: vec![],
namespace_packages: vec![],
per_file_ignores: vec![],
project_root: path_dedot::CWD.clone(),
respect_gitignore: true,
src: vec![path_dedot::CWD.clone()],
project_root: path_dedot::CWD.clone(),
tab_size: TabSize::default(),
target_version: TARGET_VERSION,
task_tags: TASK_TAGS.iter().map(ToString::to_string).collect(),
typing_modules: vec![],
Expand Down
4 changes: 3 additions & 1 deletion crates/ruff/src/settings/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,9 +101,10 @@ pub struct Settings {
pub external: FxHashSet<String>,
pub ignore_init_module_imports: bool,
pub line_length: LineLength,
pub tab_size: TabSize,
pub logger_objects: Vec<String>,
pub namespace_packages: Vec<PathBuf>,
pub src: Vec<PathBuf>,
pub tab_size: TabSize,
pub task_tags: Vec<String>,
pub typing_modules: Vec<String>,
// Plugins
Expand Down Expand Up @@ -189,6 +190,7 @@ impl Settings {
.map(ToString::to_string)
.collect()
}),
logger_objects: config.logger_objects.unwrap_or_default(),
typing_modules: config.typing_modules.unwrap_or_default(),
// Plugins
flake8_annotations: config
Expand Down
26 changes: 25 additions & 1 deletion crates/ruff/src/settings/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,30 @@ pub struct Options {
required-version = "0.0.193"
"#
)]
#[option(
default = r#"[]"#,
value_type = "list[str]",
example = r#"logger-objects = ["logging_setup.logger"]"#
)]
/// A list of objects that should be treated equivalently to a
/// `logging.Logger` object.
///
/// This is useful for ensuring proper diagnostics (e.g., to identify
/// `logging` deprecations and other best-practices) for projects that
/// re-export a `logging.Logger` object from a common module.
///
/// For example, if you have a module `logging_setup.py` with the following
/// contents:
/// ```python
/// import logging
///
/// logger = logging.getLogger(__name__)
/// ```
///
/// Adding `"logging_setup.logger"` to `logger-objects` will ensure that
/// `logging_setup.logger` is treated as a `logging.Logger` object when
/// imported from other modules (e.g., `from logging_setup import logger`).
pub logger_objects: Option<Vec<String>>,
/// Require a specific version of Ruff to be running (useful for unifying
/// results across many environments, e.g., with a `pyproject.toml`
/// file).
Expand Down Expand Up @@ -463,7 +487,7 @@ pub struct Options {
value_type = "list[str]",
example = r#"typing-modules = ["airflow.typing_compat"]"#
)]
/// A list of modules whose imports should be treated equivalently to
/// A list of modules whose exports should be treated equivalently to
/// members of the `typing` module.
///
/// This is useful for ensuring proper type annotation inference for
Expand Down
Loading

0 comments on commit f9726af

Please sign in to comment.