Skip to content

Commit

Permalink
Make it an option
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Jul 24, 2023
1 parent 3f15b33 commit 4a63aa8
Show file tree
Hide file tree
Showing 13 changed files with 159 additions and 93 deletions.
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
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 4a63aa8

Please sign in to comment.