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

feat: pylint codes E1205-E106 #3084

Merged
merged 5 commits into from
Feb 22, 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 12 additions & 0 deletions crates/ruff/resources/test/fixtures/pylint/logging_E1205.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import logging

logging.warning("Hello %s", "World!", "again") # [logging-too-many-args]

# do not handle calls with kwargs (like pylint)
logging.warning("Hello %s", "World!", "again", something="else")

logging.warning("Hello %s", "World!")

import warning

warning.warning("Hello %s", "World!", "again")
12 changes: 12 additions & 0 deletions crates/ruff/resources/test/fixtures/pylint/logging_E1206.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import logging
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: can we name these to match the rule? I think that's the convention we've been using for Pylint (e.g., logging_too_few_args.py)


logging.warning("Hello %s %s", "World!") # [logging-too-few-args]

# do not handle calls with kwargs (like pylint)
logging.warning("Hello %s", "World!", "again", something="else")

logging.warning("Hello %s", "World!")

import warning

warning.warning("Hello %s %s", "World!")
7 changes: 7 additions & 0 deletions crates/ruff/src/checkers/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2872,6 +2872,13 @@ where
{
flake8_logging_format::rules::logging_call(self, func, args, keywords);
}

// pylint logging checker
if self.settings.rules.enabled(&Rule::LoggingTooFewArgs)
|| self.settings.rules.enabled(&Rule::LoggingTooFewArgs)
md384 marked this conversation as resolved.
Show resolved Hide resolved
{
pylint::rules::logging_call(self, func, args, keywords);
}
}
ExprKind::Dict { keys, values } => {
if self
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<Rule> {
(Pylint, "E0101") => Rule::ReturnInInit,
(Pylint, "E0604") => Rule::InvalidAllObject,
(Pylint, "E0605") => Rule::InvalidAllFormat,
(Pylint, "E1205") => Rule::LoggingTooManyArgs,
(Pylint, "E1206") => Rule::LoggingTooFewArgs,
(Pylint, "E1307") => Rule::BadStringFormatType,
(Pylint, "E2502") => Rule::BidirectionalUnicode,
(Pylint, "E1310") => Rule::BadStrStripCall,
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,8 @@ ruff_macros::register_rules!(
rules::pylint::rules::TooManyArguments,
rules::pylint::rules::TooManyBranches,
rules::pylint::rules::TooManyStatements,
rules::pylint::rules::LoggingTooFewArgs,
rules::pylint::rules::LoggingTooManyArgs,
// flake8-builtins
rules::flake8_builtins::rules::BuiltinVariableShadowing,
rules::flake8_builtins::rules::BuiltinArgumentShadowing,
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff/src/rules/flake8_logging_format/rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use crate::rules::flake8_logging_format::violations::{
LoggingRedundantExcInfo, LoggingStringConcat, LoggingStringFormat, LoggingWarn,
};

enum LoggingLevel {
pub enum LoggingLevel {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this should be moved to a different file now

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move this to a new ast/logging.rs? (ast is a bit of a misnomer, but I think it's right to be alongside moduiles like ast/whitespace.rs.)

Debug,
Critical,
Error,
Expand All @@ -21,7 +21,7 @@ enum LoggingLevel {
}

impl LoggingLevel {
fn from_str(level: &str) -> Option<Self> {
pub fn from_str(level: &str) -> Option<Self> {
match level {
"debug" => Some(LoggingLevel::Debug),
"critical" => Some(LoggingLevel::Critical),
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ mod tests {
use crate::settings::Settings;
use crate::test::test_path;

#[test_case(Rule::LoggingTooManyArgs, Path::new("logging_E1205.py"); "PLE1205")]
#[test_case(Rule::LoggingTooFewArgs, Path::new("logging_E1206.py"); "PLE1206")]
#[test_case(Rule::ReturnInInit, Path::new("return_in_init.py"); "PLE0101")]
#[test_case(Rule::UselessImportAlias, Path::new("import_aliasing.py"); "PLC0414")]
#[test_case(Rule::UnnecessaryDirectLambdaCall, Path::new("unnecessary_direct_lambda_call.py"); "PLC3002")]
Expand Down
85 changes: 85 additions & 0 deletions crates/ruff/src/rules/pylint/rules/logging.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
use rustpython_parser::ast::{Constant, Expr, ExprKind, Keyword};

use ruff_macros::{define_violation, derive_message_formats};

use crate::ast::helpers::{is_logger_candidate, SimpleCallArgs};
use crate::ast::types::Range;
use crate::checkers::ast::Checker;
use crate::registry::{Diagnostic, Rule};
use crate::rules::flake8_logging_format::rules::LoggingLevel;
use crate::rules::pyflakes::cformat::CFormatSummary;
use crate::violation::Violation;

define_violation!(
pub struct LoggingTooFewArgs;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you open to writing documentation for these rules? We're trying to include documentation for all new rules. You can grep for /// ## What it does to see some examples of the expected format.

);
impl Violation for LoggingTooFewArgs {
#[derive_message_formats]
fn message(&self) -> String {
format!("Not enough arguments for logging format string")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: should we wrap logging in backticks here? Pylint may not do that, but we tend to.

}
}

define_violation!(
pub struct LoggingTooManyArgs;
);
impl Violation for LoggingTooManyArgs {
#[derive_message_formats]
fn message(&self) -> String {
format!("Too many arguments for logging format string")
}
}

/// Check logging calls for violations.
pub fn logging_call(checker: &mut Checker, func: &Expr, args: &[Expr], keywords: &[Keyword]) {
if !is_logger_candidate(func) {
return;
}

if let ExprKind::Attribute { attr, .. } = &func.node {
if let Some(_logging_level) = LoggingLevel::from_str(attr.as_str()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be .is_some() rather than a match with an unused name?

let call_args = SimpleCallArgs::new(args, keywords);

// E1205 - E1206
if let Some(msg) = call_args.get_argument("msg", Some(0)) {
if let ExprKind::Constant {
value: Constant::Str(value),
..
} = &msg.node
{
if let Ok(summary) = CFormatSummary::try_from(value.as_str()) {
if summary.starred {
return;
}

if !call_args.kwargs.is_empty() {
// Keyword checking on logging strings is complicated by
// special keywords - out of scope.
return;
}
md384 marked this conversation as resolved.
Show resolved Hide resolved

let message_args = call_args.args.len() - 1;

if checker.settings.rules.enabled(&Rule::LoggingTooFewArgs)
md384 marked this conversation as resolved.
Show resolved Hide resolved
&& summary.num_positional < message_args
{
checker.diagnostics.push(Diagnostic::new(
LoggingTooManyArgs,
Range::from_located(func),
));
}

if checker.settings.rules.enabled(&Rule::LoggingTooManyArgs)
md384 marked this conversation as resolved.
Show resolved Hide resolved
&& summary.num_positional > message_args
{
checker.diagnostics.push(Diagnostic::new(
LoggingTooFewArgs,
Range::from_located(func),
));
}
}
}
}
}
}
}
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/pylint/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ pub use consider_using_sys_exit::{consider_using_sys_exit, ConsiderUsingSysExit}
pub use global_variable_not_assigned::GlobalVariableNotAssigned;
pub use invalid_all_format::{invalid_all_format, InvalidAllFormat};
pub use invalid_all_object::{invalid_all_object, InvalidAllObject};
pub use logging::{logging_call, LoggingTooFewArgs, LoggingTooManyArgs};
pub use magic_value_comparison::{magic_value_comparison, MagicValueComparison};
pub use merge_isinstance::{merge_isinstance, ConsiderMergingIsinstance};
pub use nonlocal_without_binding::NonlocalWithoutBinding;
Expand Down Expand Up @@ -36,6 +37,7 @@ mod consider_using_sys_exit;
mod global_variable_not_assigned;
mod invalid_all_format;
mod invalid_all_object;
mod logging;
mod magic_value_comparison;
mod merge_isinstance;
mod nonlocal_without_binding;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
source: crates/ruff/src/rules/pylint/mod.rs
expression: diagnostics
---
[]

Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
source: crates/ruff/src/rules/pylint/mod.rs
expression: diagnostics
---
[]

4 changes: 4 additions & 0 deletions ruff.schema.json

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