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

Allow specification of logging.Logger re-exports via logger-objects #5750

Merged
merged 2 commits into from
Jul 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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
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
Loading