From f3b1a14cacb9b72b5b174c66cd15fbb39a4b5465 Mon Sep 17 00:00:00 2001 From: Steve C Date: Sun, 21 Jan 2024 17:33:59 -0500 Subject: [PATCH 01/18] [`pylint`] - Implement `too-few-public-methods` rule (`PLR0903`) --- ...ow_settings__display_default_settings.snap | 1 + .../fixtures/pylint/too_few_public_methods.py | 15 ++++ .../src/checkers/ast/analyze/statement.rs | 7 ++ crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/pylint/mod.rs | 16 ++++ .../ruff_linter/src/rules/pylint/rules/mod.rs | 2 + .../pylint/rules/too_few_public_methods.rs | 82 +++++++++++++++++++ .../ruff_linter/src/rules/pylint/settings.rs | 3 + ...pylint__tests__too_few_public_methods.snap | 13 +++ crates/ruff_workspace/src/configuration.rs | 2 +- ruff.schema.json | 10 +++ 11 files changed, 151 insertions(+), 1 deletion(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/too_few_public_methods.py create mode 100644 crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__too_few_public_methods.snap diff --git a/crates/ruff/tests/snapshots/show_settings__display_default_settings.snap b/crates/ruff/tests/snapshots/show_settings__display_default_settings.snap index 46cd4b0dddbae..59a4f2d0f45fa 100644 --- a/crates/ruff/tests/snapshots/show_settings__display_default_settings.snap +++ b/crates/ruff/tests/snapshots/show_settings__display_default_settings.snap @@ -371,6 +371,7 @@ linter.pylint.max_bool_expr = 5 linter.pylint.max_branches = 12 linter.pylint.max_statements = 50 linter.pylint.max_public_methods = 20 +linter.pylint.min_public_methods = 2 linter.pylint.max_locals = 15 linter.pyupgrade.keep_runtime_typing = false linter.ruff.parenthesize_tuple_in_subscript = false diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/too_few_public_methods.py b/crates/ruff_linter/resources/test/fixtures/pylint/too_few_public_methods.py new file mode 100644 index 0000000000000..603459b23e0ab --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/too_few_public_methods.py @@ -0,0 +1,15 @@ +from __future__ import annotations + + +class Point: # PLR0903 + def __init__(self, x: float, y: float) -> None: + self.x = x + self.y = y + + +class Rectangle: # OK + def __init__(self, top_left: Point, bottom_right: Point) -> None: + ... + + def area(self) -> float: + ... diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index a780c4e3ab5a1..7406c90d197ca 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -431,6 +431,13 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::EqWithoutHash) { pylint::rules::object_without_hash_method(checker, class_def); } + if checker.enabled(Rule::TooFewPublicMethods) { + pylint::rules::too_few_public_methods( + checker, + class_def, + checker.settings.pylint.min_public_methods, + ); + } if checker.enabled(Rule::TooManyPublicMethods) { pylint::rules::too_many_public_methods( checker, diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 30bc6949f8c88..fe4a9728a8b71 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -293,6 +293,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "W1514") => (RuleGroup::Preview, rules::pylint::rules::UnspecifiedEncoding), (Pylint, "W1641") => (RuleGroup::Preview, rules::pylint::rules::EqWithoutHash), (Pylint, "W2101") => (RuleGroup::Stable, rules::pylint::rules::UselessWithLock), + (Pylint, "R0903") => (RuleGroup::Preview, rules::pylint::rules::TooFewPublicMethods), (Pylint, "W2901") => (RuleGroup::Stable, rules::pylint::rules::RedefinedLoopName), (Pylint, "W3201") => (RuleGroup::Preview, rules::pylint::rules::BadDunderMethodName), (Pylint, "W3301") => (RuleGroup::Stable, rules::pylint::rules::NestedMinMax), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 784f3f89954a9..17c00c76234ee 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -392,6 +392,22 @@ mod tests { Ok(()) } + #[test] + fn too_few_public_methods() -> Result<()> { + let diagnostics = test_path( + Path::new("pylint/too_few_public_methods.py"), + &LinterSettings { + pylint: pylint::settings::Settings { + min_public_methods: 2, + ..pylint::settings::Settings::default() + }, + ..LinterSettings::for_rules(vec![Rule::TooFewPublicMethods]) + }, + )?; + assert_messages!(diagnostics); + Ok(()) + } + #[test] fn too_many_locals() -> Result<()> { let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 8d019d0887a48..cf1892193502c 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -77,6 +77,7 @@ pub(crate) use subprocess_popen_preexec_fn::*; pub(crate) use subprocess_run_without_check::*; pub(crate) use super_without_brackets::*; pub(crate) use sys_exit_alias::*; +pub(crate) use too_few_public_methods::*; pub(crate) use too_many_arguments::*; pub(crate) use too_many_boolean_expressions::*; pub(crate) use too_many_branches::*; @@ -184,6 +185,7 @@ mod subprocess_popen_preexec_fn; mod subprocess_run_without_check; mod super_without_brackets; mod sys_exit_alias; +mod too_few_public_methods; mod too_many_arguments; mod too_many_boolean_expressions; mod too_many_branches; diff --git a/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs b/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs new file mode 100644 index 0000000000000..e47643adf1587 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs @@ -0,0 +1,82 @@ +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast as ast; +use ruff_python_semantic::analyze::visibility::{self, Visibility::Public}; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for classes with too few public methods +/// +/// By default, this rule allows down to 2 public methods, as configured by +/// the [`pylint.min-public-methods`] option. +/// +/// ## Why is this bad? +/// Classes with too few public methods are possibly better off +/// being a dataclass or a namedtuple, which are more lightweight. +/// +/// ## Example +/// Assuming that `pylint.min-public-settings` is set to 2: +/// ```python +/// class Point: +/// def __init__(self, x: float, y: float): +/// self.x = x +/// self.y = y +/// ``` +/// +/// Use instead: +/// ```python +/// from dataclasses import dataclass +/// +/// +/// @dataclass +/// class Point: +/// x: float +/// y: float +/// ``` +/// +/// ## Options +/// - `pylint.min-public-methods` +#[violation] +pub struct TooFewPublicMethods { + methods: usize, + min_methods: usize, +} + +impl Violation for TooFewPublicMethods { + #[derive_message_formats] + fn message(&self) -> String { + let TooFewPublicMethods { + methods, + min_methods, + } = self; + format!("Too few public methods ({methods} < {min_methods})") + } +} + +/// R0903 +pub(crate) fn too_few_public_methods( + checker: &mut Checker, + class_def: &ast::StmtClassDef, + min_methods: usize, +) { + let methods = class_def + .body + .iter() + .filter(|stmt| { + stmt.as_function_def_stmt() + .is_some_and(|node| matches!(visibility::method_visibility(node), Public)) + }) + .count(); + + if methods < min_methods { + checker.diagnostics.push(Diagnostic::new( + TooFewPublicMethods { + methods, + min_methods, + }, + class_def.range(), + )); + } +} diff --git a/crates/ruff_linter/src/rules/pylint/settings.rs b/crates/ruff_linter/src/rules/pylint/settings.rs index 383f5136c8de0..4bdf2bf0ce998 100644 --- a/crates/ruff_linter/src/rules/pylint/settings.rs +++ b/crates/ruff_linter/src/rules/pylint/settings.rs @@ -59,6 +59,7 @@ pub struct Settings { pub max_branches: usize, pub max_statements: usize, pub max_public_methods: usize, + pub min_public_methods: usize, pub max_locals: usize, pub max_nested_blocks: usize, } @@ -75,6 +76,7 @@ impl Default for Settings { max_branches: 12, max_statements: 50, max_public_methods: 20, + min_public_methods: 2, max_locals: 15, max_nested_blocks: 5, } @@ -96,6 +98,7 @@ impl fmt::Display for Settings { self.max_branches, self.max_statements, self.max_public_methods, + self.min_public_methods, self.max_locals ] } diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__too_few_public_methods.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__too_few_public_methods.snap new file mode 100644 index 0000000000000..57d8796931822 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__too_few_public_methods.snap @@ -0,0 +1,13 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +too_few_public_methods.py:4:1: PLR0903 Too few public methods (1 < 2) + | +4 | / class Point: # PLR0903 +5 | | def __init__(self, x: float, y: float) -> None: +6 | | self.x = x +7 | | self.y = y + | |__________________^ PLR0903 + | + + diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index 56b8f479dfb35..c3eca2429866d 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -1571,7 +1571,7 @@ mod tests { const PREVIEW_RULES: &[Rule] = &[ Rule::ReimplementedStarmap, Rule::SliceCopy, - Rule::TooManyPublicMethods, + Rule::TooFewPublicMethods, Rule::TooManyPublicMethods, Rule::UnnecessaryEnumerate, Rule::MathConstant, diff --git a/ruff.schema.json b/ruff.schema.json index b894dbb692d0d..709deabb6a5d4 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2699,6 +2699,15 @@ ], "format": "uint", "minimum": 0.0 + }, + "min-public-methods": { + "description": "Minimum number of public methods allowed within a single class (see: `PLR0903`).", + "type": [ + "integer", + "null" + ], + "format": "uint", + "minimum": 0.0 } }, "additionalProperties": false @@ -3559,6 +3568,7 @@ "PLR0402", "PLR09", "PLR090", + "PLR0903", "PLR0904", "PLR091", "PLR0911", From 6ccd4a0c155e00503e12532ec45d50bf156d4a32 Mon Sep 17 00:00:00 2001 From: Steve C Date: Sun, 21 Jan 2024 19:30:20 -0500 Subject: [PATCH 02/18] allow decorated and subclasses --- .../fixtures/pylint/too_few_public_methods.py | 15 +++++++++++++++ .../rules/pylint/rules/too_few_public_methods.rs | 10 ++++++++++ ...es__pylint__tests__too_few_public_methods.snap | 10 +++++----- 3 files changed, 30 insertions(+), 5 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/too_few_public_methods.py b/crates/ruff_linter/resources/test/fixtures/pylint/too_few_public_methods.py index 603459b23e0ab..67f1089ccc8dc 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/too_few_public_methods.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/too_few_public_methods.py @@ -1,5 +1,7 @@ from __future__ import annotations +from dataclasses import dataclass + class Point: # PLR0903 def __init__(self, x: float, y: float) -> None: @@ -13,3 +15,16 @@ def __init__(self, top_left: Point, bottom_right: Point) -> None: def area(self) -> float: ... + + +@dataclass +class Circle: # OK + center: Point + radius: float + + def area(self) -> float: + ... + + +class CustomException(Exception): # OK + ... diff --git a/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs b/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs index e47643adf1587..6d7c35acda89d 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs @@ -61,6 +61,16 @@ pub(crate) fn too_few_public_methods( class_def: &ast::StmtClassDef, min_methods: usize, ) { + // allow decorated classes + if !class_def.decorator_list.is_empty() { + return; + } + + // allow classes with base classes + if class_def.arguments.is_some() { + return; + } + let methods = class_def .body .iter() diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__too_few_public_methods.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__too_few_public_methods.snap index 57d8796931822..4822f5e3e411f 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__too_few_public_methods.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__too_few_public_methods.snap @@ -1,12 +1,12 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -too_few_public_methods.py:4:1: PLR0903 Too few public methods (1 < 2) +too_few_public_methods.py:6:1: PLR0903 Too few public methods (1 < 2) | -4 | / class Point: # PLR0903 -5 | | def __init__(self, x: float, y: float) -> None: -6 | | self.x = x -7 | | self.y = y +6 | / class Point: # PLR0903 +7 | | def __init__(self, x: float, y: float) -> None: +8 | | self.x = x +9 | | self.y = y | |__________________^ PLR0903 | From 9dd59a31645cca86e70658e3bc9b958834f74322 Mon Sep 17 00:00:00 2001 From: Steve C Date: Wed, 24 Jan 2024 20:46:38 -0500 Subject: [PATCH 03/18] suggested tweaks --- ...ow_settings__display_default_settings.snap | 1 - .../fixtures/pylint/too_few_public_methods.py | 8 ++ .../src/checkers/ast/analyze/statement.rs | 6 +- crates/ruff_linter/src/rules/pylint/mod.rs | 17 +---- .../pylint/rules/too_few_public_methods.rs | 73 +++++++++---------- .../ruff_linter/src/rules/pylint/settings.rs | 3 - ...s__PLR0903_too_few_public_methods.py.snap} | 2 +- ruff.schema.json | 9 --- 8 files changed, 45 insertions(+), 74 deletions(-) rename crates/ruff_linter/src/rules/pylint/snapshots/{ruff_linter__rules__pylint__tests__too_few_public_methods.snap => ruff_linter__rules__pylint__tests__PLR0903_too_few_public_methods.py.snap} (75%) diff --git a/crates/ruff/tests/snapshots/show_settings__display_default_settings.snap b/crates/ruff/tests/snapshots/show_settings__display_default_settings.snap index 59a4f2d0f45fa..46cd4b0dddbae 100644 --- a/crates/ruff/tests/snapshots/show_settings__display_default_settings.snap +++ b/crates/ruff/tests/snapshots/show_settings__display_default_settings.snap @@ -371,7 +371,6 @@ linter.pylint.max_bool_expr = 5 linter.pylint.max_branches = 12 linter.pylint.max_statements = 50 linter.pylint.max_public_methods = 20 -linter.pylint.min_public_methods = 2 linter.pylint.max_locals = 15 linter.pyupgrade.keep_runtime_typing = false linter.ruff.parenthesize_tuple_in_subscript = false diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/too_few_public_methods.py b/crates/ruff_linter/resources/test/fixtures/pylint/too_few_public_methods.py index 67f1089ccc8dc..2262460a4ed7d 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/too_few_public_methods.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/too_few_public_methods.py @@ -28,3 +28,11 @@ def area(self) -> float: class CustomException(Exception): # OK ... + + +class A: + class B: + ... + + def __init__(self): + ... diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 7406c90d197ca..653a81f788023 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -432,11 +432,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { pylint::rules::object_without_hash_method(checker, class_def); } if checker.enabled(Rule::TooFewPublicMethods) { - pylint::rules::too_few_public_methods( - checker, - class_def, - checker.settings.pylint.min_public_methods, - ); + pylint::rules::too_few_public_methods(checker, class_def); } if checker.enabled(Rule::TooManyPublicMethods) { pylint::rules::too_many_public_methods( diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 17c00c76234ee..d79f09f00b083 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -207,6 +207,7 @@ mod tests { #[test_case(Rule::TooManyNestedBlocks, Path::new("too_many_nested_blocks.py"))] #[test_case(Rule::DictIndexMissingItems, Path::new("dict_index_missing_items.py"))] #[test_case(Rule::DictIterMissingItems, Path::new("dict_iter_missing_items.py"))] + #[test_case(Rule::TooFewPublicMethods, Path::new("too_few_public_methods.py"))] #[test_case( Rule::UnnecessaryDictIndexLookup, Path::new("unnecessary_dict_index_lookup.py") @@ -392,22 +393,6 @@ mod tests { Ok(()) } - #[test] - fn too_few_public_methods() -> Result<()> { - let diagnostics = test_path( - Path::new("pylint/too_few_public_methods.py"), - &LinterSettings { - pylint: pylint::settings::Settings { - min_public_methods: 2, - ..pylint::settings::Settings::default() - }, - ..LinterSettings::for_rules(vec![Rule::TooFewPublicMethods]) - }, - )?; - assert_messages!(diagnostics); - Ok(()) - } - #[test] fn too_many_locals() -> Result<()> { let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs b/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs index 6d7c35acda89d..9298a3dd58f33 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs @@ -7,17 +7,14 @@ use ruff_text_size::Ranged; use crate::checkers::ast::Checker; /// ## What it does -/// Checks for classes with too few public methods -/// -/// By default, this rule allows down to 2 public methods, as configured by -/// the [`pylint.min-public-methods`] option. +/// Checks for classes that only have a public `__init__` method, +/// as well as 0 base classes, and 0 decorators. /// /// ## Why is this bad? -/// Classes with too few public methods are possibly better off +/// Classes with just an `__init__` are possibly better off /// being a dataclass or a namedtuple, which are more lightweight. /// /// ## Example -/// Assuming that `pylint.min-public-settings` is set to 2: /// ```python /// class Point: /// def __init__(self, x: float, y: float): @@ -35,32 +32,18 @@ use crate::checkers::ast::Checker; /// x: float /// y: float /// ``` -/// -/// ## Options -/// - `pylint.min-public-methods` #[violation] -pub struct TooFewPublicMethods { - methods: usize, - min_methods: usize, -} +pub struct TooFewPublicMethods; impl Violation for TooFewPublicMethods { #[derive_message_formats] fn message(&self) -> String { - let TooFewPublicMethods { - methods, - min_methods, - } = self; - format!("Too few public methods ({methods} < {min_methods})") + format!("Class could be dataclass or namedtuple") } } /// R0903 -pub(crate) fn too_few_public_methods( - checker: &mut Checker, - class_def: &ast::StmtClassDef, - min_methods: usize, -) { +pub(crate) fn too_few_public_methods(checker: &mut Checker, class_def: &ast::StmtClassDef) { // allow decorated classes if !class_def.decorator_list.is_empty() { return; @@ -71,22 +54,34 @@ pub(crate) fn too_few_public_methods( return; } - let methods = class_def - .body - .iter() - .filter(|stmt| { - stmt.as_function_def_stmt() - .is_some_and(|node| matches!(visibility::method_visibility(node), Public)) - }) - .count(); + let mut public_methods = 0; + let mut has_dunder_init = false; + + for stmt in &class_def.body { + if public_methods > 1 && has_dunder_init { + // we're good to break here + break; + } + match stmt { + ast::Stmt::FunctionDef(node) => { + if !has_dunder_init && node.name.to_string() == "__init__" { + has_dunder_init = true; + } + if matches!(visibility::method_visibility(node), Public) { + public_methods += 1; + } + } + ast::Stmt::ClassDef(_) => { + // allow classes with nested classes, often used for config + return; + } + _ => {} + } + } - if methods < min_methods { - checker.diagnostics.push(Diagnostic::new( - TooFewPublicMethods { - methods, - min_methods, - }, - class_def.range(), - )); + if has_dunder_init && public_methods == 1 { + checker + .diagnostics + .push(Diagnostic::new(TooFewPublicMethods, class_def.range())); } } diff --git a/crates/ruff_linter/src/rules/pylint/settings.rs b/crates/ruff_linter/src/rules/pylint/settings.rs index 4bdf2bf0ce998..383f5136c8de0 100644 --- a/crates/ruff_linter/src/rules/pylint/settings.rs +++ b/crates/ruff_linter/src/rules/pylint/settings.rs @@ -59,7 +59,6 @@ pub struct Settings { pub max_branches: usize, pub max_statements: usize, pub max_public_methods: usize, - pub min_public_methods: usize, pub max_locals: usize, pub max_nested_blocks: usize, } @@ -76,7 +75,6 @@ impl Default for Settings { max_branches: 12, max_statements: 50, max_public_methods: 20, - min_public_methods: 2, max_locals: 15, max_nested_blocks: 5, } @@ -98,7 +96,6 @@ impl fmt::Display for Settings { self.max_branches, self.max_statements, self.max_public_methods, - self.min_public_methods, self.max_locals ] } diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__too_few_public_methods.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0903_too_few_public_methods.py.snap similarity index 75% rename from crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__too_few_public_methods.snap rename to crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0903_too_few_public_methods.py.snap index 4822f5e3e411f..463e3505222bd 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__too_few_public_methods.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0903_too_few_public_methods.py.snap @@ -1,7 +1,7 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -too_few_public_methods.py:6:1: PLR0903 Too few public methods (1 < 2) +too_few_public_methods.py:6:1: PLR0903 Class could be dataclass or namedtuple | 6 | / class Point: # PLR0903 7 | | def __init__(self, x: float, y: float) -> None: diff --git a/ruff.schema.json b/ruff.schema.json index 709deabb6a5d4..b8125949f86c7 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2699,15 +2699,6 @@ ], "format": "uint", "minimum": 0.0 - }, - "min-public-methods": { - "description": "Minimum number of public methods allowed within a single class (see: `PLR0903`).", - "type": [ - "integer", - "null" - ], - "format": "uint", - "minimum": 0.0 } }, "additionalProperties": false From ffa980328a4b21f5973c4b8d2668d3bc4334c780 Mon Sep 17 00:00:00 2001 From: Steve C Date: Fri, 15 Mar 2024 00:56:11 -0400 Subject: [PATCH 04/18] tweak "what it does" wording Co-authored-by: Micha Reiser --- .../src/rules/pylint/rules/too_few_public_methods.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs b/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs index 9298a3dd58f33..c522e2dc00559 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs @@ -8,7 +8,7 @@ use crate::checkers::ast::Checker; /// ## What it does /// Checks for classes that only have a public `__init__` method, -/// as well as 0 base classes, and 0 decorators. +/// without base classes and 0 decorators. /// /// ## Why is this bad? /// Classes with just an `__init__` are possibly better off From c1701f2ae85bf65ffcb6b6007713a532803e80a9 Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Fri, 3 Jan 2025 15:04:33 -0600 Subject: [PATCH 05/18] use derive trait for violation --- .../src/rules/pylint/rules/too_few_public_methods.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs b/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs index c522e2dc00559..2492fb3f08210 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs @@ -1,5 +1,5 @@ use ruff_diagnostics::{Diagnostic, Violation}; -use ruff_macros::{derive_message_formats, violation}; +use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_ast as ast; use ruff_python_semantic::analyze::visibility::{self, Visibility::Public}; use ruff_text_size::Ranged; @@ -32,13 +32,13 @@ use crate::checkers::ast::Checker; /// x: float /// y: float /// ``` -#[violation] -pub struct TooFewPublicMethods; +#[derive(ViolationMetadata)] +pub(crate) struct TooFewPublicMethods; impl Violation for TooFewPublicMethods { #[derive_message_formats] fn message(&self) -> String { - format!("Class could be dataclass or namedtuple") + "Class could be dataclass or namedtuple".to_string() } } From 80282c3c5012bc6ad528f24e47260431c94a30c6 Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Fri, 3 Jan 2025 15:06:00 -0600 Subject: [PATCH 06/18] wording of docstring --- .../src/rules/pylint/rules/too_few_public_methods.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs b/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs index 2492fb3f08210..f305f8d86bac3 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs @@ -12,7 +12,7 @@ use crate::checkers::ast::Checker; /// /// ## Why is this bad? /// Classes with just an `__init__` are possibly better off -/// being a dataclass or a namedtuple, which are more lightweight. +/// being a dataclass or a namedtuple, which have less boilerplate. /// /// ## Example /// ```python From 4a5a8c26c9520fa3912ee54f66fa899afffdadb3 Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Fri, 3 Jan 2025 15:25:33 -0600 Subject: [PATCH 07/18] only lint if init args are non variadic and have annotations --- .../rules/pylint/rules/too_few_public_methods.rs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs b/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs index f305f8d86bac3..d06cb408a2735 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs @@ -63,11 +63,19 @@ pub(crate) fn too_few_public_methods(checker: &mut Checker, class_def: &ast::Stm break; } match stmt { - ast::Stmt::FunctionDef(node) => { - if !has_dunder_init && node.name.to_string() == "__init__" { + ast::Stmt::FunctionDef(func_def) => { + if !has_dunder_init + && func_def.name.to_string() == "__init__" + && func_def + .parameters + .iter() + // skip `self` + .skip(1) + .all(|param| param.annotation().is_some() && !param.is_variadic()) + { has_dunder_init = true; } - if matches!(visibility::method_visibility(node), Public) { + if matches!(visibility::method_visibility(func_def), Public) { public_methods += 1; } } From e648148a5b31ea3ae044885d3133b7d4ecbe01df Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Fri, 3 Jan 2025 15:25:47 -0600 Subject: [PATCH 08/18] couple more test cases --- .../fixtures/pylint/too_few_public_methods.py | 9 ++++++++ ...ts__PLR0903_too_few_public_methods.py.snap | 21 +++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/too_few_public_methods.py b/crates/ruff_linter/resources/test/fixtures/pylint/too_few_public_methods.py index 2262460a4ed7d..ac720da8f50aa 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/too_few_public_methods.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/too_few_public_methods.py @@ -36,3 +36,12 @@ class B: def __init__(self): ... + +class C: + c: int + def __init__(self,d:list):... + +class D: + """This class has a docstring.""" + # this next method is an init + def __init__(self,e:dict):... diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0903_too_few_public_methods.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0903_too_few_public_methods.py.snap index 463e3505222bd..adeeaaa9cb5ea 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0903_too_few_public_methods.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0903_too_few_public_methods.py.snap @@ -10,4 +10,25 @@ too_few_public_methods.py:6:1: PLR0903 Class could be dataclass or namedtuple | |__________________^ PLR0903 | +too_few_public_methods.py:40:1: PLR0903 Class could be dataclass or namedtuple + | +38 | ... +39 | +40 | / class C: +41 | | c: int +42 | | def __init__(self,d:list):... + | |_________________________________^ PLR0903 +43 | +44 | class D: + | +too_few_public_methods.py:44:1: PLR0903 Class could be dataclass or namedtuple + | +42 | def __init__(self,d:list):... +43 | +44 | / class D: +45 | | """This class has a docstring.""" +46 | | # this next method is an init +47 | | def __init__(self,e:dict):... + | |_________________________________^ PLR0903 + | From a4e6f5e2211918e593f9cab6cfc8f9e6c704bea4 Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Fri, 3 Jan 2025 15:30:15 -0600 Subject: [PATCH 09/18] rename rule --- crates/ruff_linter/src/checkers/ast/analyze/statement.rs | 2 +- crates/ruff_linter/src/codes.rs | 2 +- ...o_few_public_methods.rs => class_as_data_structure.rs} | 8 ++++---- crates/ruff_linter/src/rules/pylint/rules/mod.rs | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) rename crates/ruff_linter/src/rules/pylint/rules/{too_few_public_methods.rs => class_as_data_structure.rs} (90%) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 653a81f788023..22e9ae7371777 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -432,7 +432,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { pylint::rules::object_without_hash_method(checker, class_def); } if checker.enabled(Rule::TooFewPublicMethods) { - pylint::rules::too_few_public_methods(checker, class_def); + pylint::rules::class_as_data_structure(checker, class_def); } if checker.enabled(Rule::TooManyPublicMethods) { pylint::rules::too_many_public_methods( diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index fe4a9728a8b71..c09ad0a11b50f 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -293,7 +293,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "W1514") => (RuleGroup::Preview, rules::pylint::rules::UnspecifiedEncoding), (Pylint, "W1641") => (RuleGroup::Preview, rules::pylint::rules::EqWithoutHash), (Pylint, "W2101") => (RuleGroup::Stable, rules::pylint::rules::UselessWithLock), - (Pylint, "R0903") => (RuleGroup::Preview, rules::pylint::rules::TooFewPublicMethods), + (Pylint, "R0903") => (RuleGroup::Preview, rules::pylint::rules::ClassAsDataStructure), (Pylint, "W2901") => (RuleGroup::Stable, rules::pylint::rules::RedefinedLoopName), (Pylint, "W3201") => (RuleGroup::Preview, rules::pylint::rules::BadDunderMethodName), (Pylint, "W3301") => (RuleGroup::Stable, rules::pylint::rules::NestedMinMax), diff --git a/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs b/crates/ruff_linter/src/rules/pylint/rules/class_as_data_structure.rs similarity index 90% rename from crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs rename to crates/ruff_linter/src/rules/pylint/rules/class_as_data_structure.rs index d06cb408a2735..17add9cb8c87e 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/too_few_public_methods.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/class_as_data_structure.rs @@ -33,9 +33,9 @@ use crate::checkers::ast::Checker; /// y: float /// ``` #[derive(ViolationMetadata)] -pub(crate) struct TooFewPublicMethods; +pub(crate) struct ClassAsDataStructure; -impl Violation for TooFewPublicMethods { +impl Violation for ClassAsDataStructure { #[derive_message_formats] fn message(&self) -> String { "Class could be dataclass or namedtuple".to_string() @@ -43,7 +43,7 @@ impl Violation for TooFewPublicMethods { } /// R0903 -pub(crate) fn too_few_public_methods(checker: &mut Checker, class_def: &ast::StmtClassDef) { +pub(crate) fn class_as_data_structure(checker: &mut Checker, class_def: &ast::StmtClassDef) { // allow decorated classes if !class_def.decorator_list.is_empty() { return; @@ -90,6 +90,6 @@ pub(crate) fn too_few_public_methods(checker: &mut Checker, class_def: &ast::Stm if has_dunder_init && public_methods == 1 { checker .diagnostics - .push(Diagnostic::new(TooFewPublicMethods, class_def.range())); + .push(Diagnostic::new(ClassAsDataStructure, class_def.range())); } } diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index cf1892193502c..14a9da98da16f 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -10,6 +10,7 @@ pub(crate) use bad_string_format_type::*; pub(crate) use bidirectional_unicode::*; pub(crate) use binary_op_exception::*; pub(crate) use boolean_chained_comparison::*; +pub(crate) use class_as_data_structure::*; pub(crate) use collapsible_else_if::*; pub(crate) use compare_to_empty_string::*; pub(crate) use comparison_of_constant::*; @@ -77,7 +78,6 @@ pub(crate) use subprocess_popen_preexec_fn::*; pub(crate) use subprocess_run_without_check::*; pub(crate) use super_without_brackets::*; pub(crate) use sys_exit_alias::*; -pub(crate) use too_few_public_methods::*; pub(crate) use too_many_arguments::*; pub(crate) use too_many_boolean_expressions::*; pub(crate) use too_many_branches::*; @@ -118,6 +118,7 @@ mod bad_string_format_type; mod bidirectional_unicode; mod binary_op_exception; mod boolean_chained_comparison; +mod class_as_data_structure; mod collapsible_else_if; mod compare_to_empty_string; mod comparison_of_constant; @@ -185,7 +186,6 @@ mod subprocess_popen_preexec_fn; mod subprocess_run_without_check; mod super_without_brackets; mod sys_exit_alias; -mod too_few_public_methods; mod too_many_arguments; mod too_many_boolean_expressions; mod too_many_branches; From bb0f0e7c773ffbedbe594cd80f0b666e4eca6219 Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Fri, 3 Jan 2025 15:42:38 -0600 Subject: [PATCH 10/18] a few more rename spots --- crates/ruff_linter/src/checkers/ast/analyze/statement.rs | 2 +- crates/ruff_linter/src/rules/pylint/mod.rs | 2 +- crates/ruff_workspace/src/configuration.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 22e9ae7371777..676559cc0c291 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -431,7 +431,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::EqWithoutHash) { pylint::rules::object_without_hash_method(checker, class_def); } - if checker.enabled(Rule::TooFewPublicMethods) { + if checker.enabled(Rule::ClassAsDataStructure) { pylint::rules::class_as_data_structure(checker, class_def); } if checker.enabled(Rule::TooManyPublicMethods) { diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index d79f09f00b083..939b512a10ee5 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -207,7 +207,7 @@ mod tests { #[test_case(Rule::TooManyNestedBlocks, Path::new("too_many_nested_blocks.py"))] #[test_case(Rule::DictIndexMissingItems, Path::new("dict_index_missing_items.py"))] #[test_case(Rule::DictIterMissingItems, Path::new("dict_iter_missing_items.py"))] - #[test_case(Rule::TooFewPublicMethods, Path::new("too_few_public_methods.py"))] + #[test_case(Rule::ClassAsDataStructure, Path::new("too_few_public_methods.py"))] #[test_case( Rule::UnnecessaryDictIndexLookup, Path::new("unnecessary_dict_index_lookup.py") diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index c3eca2429866d..22436db9c92ae 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -1571,7 +1571,7 @@ mod tests { const PREVIEW_RULES: &[Rule] = &[ Rule::ReimplementedStarmap, Rule::SliceCopy, - Rule::TooFewPublicMethods, + Rule::ClassAsDataStructure, Rule::TooManyPublicMethods, Rule::UnnecessaryEnumerate, Rule::MathConstant, From 24a8bf36e5aabd5dbc14e75e6d322152972948af Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Fri, 3 Jan 2025 15:47:57 -0600 Subject: [PATCH 11/18] add flake8 tests and rename fixture --- .../pylint/class_as_data_structure.py | 93 +++++++++++++++++++ .../fixtures/pylint/too_few_public_methods.py | 47 ---------- 2 files changed, 93 insertions(+), 47 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/class_as_data_structure.py delete mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/too_few_public_methods.py diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/class_as_data_structure.py b/crates/ruff_linter/resources/test/fixtures/pylint/class_as_data_structure.py new file mode 100644 index 0000000000000..ce81ce5eedfc3 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/class_as_data_structure.py @@ -0,0 +1,93 @@ +from __future__ import annotations + +from dataclasses import dataclass + + +class Point: # PLR0903 + def __init__(self, x: float, y: float) -> None: + self.x = x + self.y = y + + +class Rectangle: # OK + def __init__(self, top_left: Point, bottom_right: Point) -> None: + ... + + def area(self) -> float: + ... + + +@dataclass +class Circle: # OK + center: Point + radius: float + + def area(self) -> float: + ... + + +class CustomException(Exception): # OK + ... + + +class A: + class B: + ... + + def __init__(self): + ... + +class C: + c: int + def __init__(self,d:list):... + +class D: + """This class has a docstring.""" + # this next method is an init + def __init__(self,e:dict):... + +# <--- begin flake8-bugbear tests below +class NoWarningsMoreMethods: + def __init__(self, foo, bar): + self.foo = foo + self.bar = bar + + def other_function(self): ... + + +class NoWarningsClassAttributes: + spam = "ham" + + def __init__(self, foo, bar): + self.foo = foo + self.bar = bar + + +class NoWarningsComplicatedAssignment: + def __init__(self, foo, bar): + self.foo = foo + self.bar = bar + self.spam = " - ".join([foo, bar]) + + +class NoWarningsMoreStatements: + def __init__(self, foo, bar): + foo = " - ".join([foo, bar]) + self.foo = foo + self.bar = bar + + +class Warnings: + def __init__(self, foo, bar): + self.foo = foo + self.bar = bar + + +class WarningsWithDocstring: + """A docstring should not be an impediment to a warning""" + + def __init__(self, foo, bar): + self.foo = foo + self.bar = bar + +# <-- end flake8-bugbear tests diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/too_few_public_methods.py b/crates/ruff_linter/resources/test/fixtures/pylint/too_few_public_methods.py deleted file mode 100644 index ac720da8f50aa..0000000000000 --- a/crates/ruff_linter/resources/test/fixtures/pylint/too_few_public_methods.py +++ /dev/null @@ -1,47 +0,0 @@ -from __future__ import annotations - -from dataclasses import dataclass - - -class Point: # PLR0903 - def __init__(self, x: float, y: float) -> None: - self.x = x - self.y = y - - -class Rectangle: # OK - def __init__(self, top_left: Point, bottom_right: Point) -> None: - ... - - def area(self) -> float: - ... - - -@dataclass -class Circle: # OK - center: Point - radius: float - - def area(self) -> float: - ... - - -class CustomException(Exception): # OK - ... - - -class A: - class B: - ... - - def __init__(self): - ... - -class C: - c: int - def __init__(self,d:list):... - -class D: - """This class has a docstring.""" - # this next method is an init - def __init__(self,e:dict):... From 330188242a6b440215104e0aba5cba41685582e6 Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Fri, 3 Jan 2025 16:25:46 -0600 Subject: [PATCH 12/18] move rule to bugbear --- .../class_as_data_structure.py | 15 ++-- .../src/checkers/ast/analyze/statement.rs | 2 +- crates/ruff_linter/src/codes.rs | 2 +- .../src/rules/flake8_bugbear/mod.rs | 1 + .../rules/class_as_data_structure.rs | 2 +- .../src/rules/flake8_bugbear/rules/mod.rs | 2 + ...sts__BB903_class_as_data_structure.py.snap | 89 +++++++++++++++++++ crates/ruff_linter/src/rules/pylint/mod.rs | 1 - .../ruff_linter/src/rules/pylint/rules/mod.rs | 2 - ...ts__PLR0903_too_few_public_methods.py.snap | 34 ------- ruff.schema.json | 5 +- 11 files changed, 108 insertions(+), 47 deletions(-) rename crates/ruff_linter/resources/test/fixtures/{pylint => flake8_bugbear}/class_as_data_structure.py (79%) rename crates/ruff_linter/src/rules/{pylint => flake8_bugbear}/rules/class_as_data_structure.rs (99%) create mode 100644 crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__BB903_class_as_data_structure.py.snap delete mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0903_too_few_public_methods.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/class_as_data_structure.py b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/class_as_data_structure.py similarity index 79% rename from crates/ruff_linter/resources/test/fixtures/pylint/class_as_data_structure.py rename to crates/ruff_linter/resources/test/fixtures/flake8_bugbear/class_as_data_structure.py index ce81ce5eedfc3..67d8f1c4f892e 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/class_as_data_structure.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/class_as_data_structure.py @@ -47,8 +47,11 @@ class D: def __init__(self,e:dict):... # <--- begin flake8-bugbear tests below +# (we have modified them to have type annotations, +# since our implementation only triggers in that +# stricter setting.) class NoWarningsMoreMethods: - def __init__(self, foo, bar): + def __init__(self, foo:int, bar:list): self.foo = foo self.bar = bar @@ -58,27 +61,27 @@ def other_function(self): ... class NoWarningsClassAttributes: spam = "ham" - def __init__(self, foo, bar): + def __init__(self, foo:int, bar:list): self.foo = foo self.bar = bar class NoWarningsComplicatedAssignment: - def __init__(self, foo, bar): + def __init__(self, foo:int, bar:list): self.foo = foo self.bar = bar self.spam = " - ".join([foo, bar]) class NoWarningsMoreStatements: - def __init__(self, foo, bar): + def __init__(self, foo:int, bar:list): foo = " - ".join([foo, bar]) self.foo = foo self.bar = bar class Warnings: - def __init__(self, foo, bar): + def __init__(self, foo:int, bar:list): self.foo = foo self.bar = bar @@ -86,7 +89,7 @@ def __init__(self, foo, bar): class WarningsWithDocstring: """A docstring should not be an impediment to a warning""" - def __init__(self, foo, bar): + def __init__(self, foo:int, bar:list): self.foo = foo self.bar = bar diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 676559cc0c291..7a3479172f1b2 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -432,7 +432,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { pylint::rules::object_without_hash_method(checker, class_def); } if checker.enabled(Rule::ClassAsDataStructure) { - pylint::rules::class_as_data_structure(checker, class_def); + flake8_bugbear::rules::class_as_data_structure(checker, class_def); } if checker.enabled(Rule::TooManyPublicMethods) { pylint::rules::too_many_public_methods( diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index c09ad0a11b50f..ba866e30e674f 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -293,7 +293,6 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "W1514") => (RuleGroup::Preview, rules::pylint::rules::UnspecifiedEncoding), (Pylint, "W1641") => (RuleGroup::Preview, rules::pylint::rules::EqWithoutHash), (Pylint, "W2101") => (RuleGroup::Stable, rules::pylint::rules::UselessWithLock), - (Pylint, "R0903") => (RuleGroup::Preview, rules::pylint::rules::ClassAsDataStructure), (Pylint, "W2901") => (RuleGroup::Stable, rules::pylint::rules::RedefinedLoopName), (Pylint, "W3201") => (RuleGroup::Preview, rules::pylint::rules::BadDunderMethodName), (Pylint, "W3301") => (RuleGroup::Stable, rules::pylint::rules::NestedMinMax), @@ -357,6 +356,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8Bugbear, "035") => (RuleGroup::Stable, rules::flake8_bugbear::rules::StaticKeyDictComprehension), (Flake8Bugbear, "039") => (RuleGroup::Stable, rules::flake8_bugbear::rules::MutableContextvarDefault), (Flake8Bugbear, "901") => (RuleGroup::Preview, rules::flake8_bugbear::rules::ReturnInGenerator), + (Flake8Bugbear, "B903") => (RuleGroup::Preview, rules::flake8_bugbear::rules::ClassAsDataStructure), (Flake8Bugbear, "904") => (RuleGroup::Stable, rules::flake8_bugbear::rules::RaiseWithoutFromInsideExcept), (Flake8Bugbear, "905") => (RuleGroup::Stable, rules::flake8_bugbear::rules::ZipWithoutExplicitStrict), (Flake8Bugbear, "909") => (RuleGroup::Preview, rules::flake8_bugbear::rules::LoopIteratorMutation), diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs b/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs index b254847c38937..27510d7e9282d 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs @@ -21,6 +21,7 @@ mod tests { #[test_case(Rule::AssertRaisesException, Path::new("B017.py"))] #[test_case(Rule::AssignmentToOsEnviron, Path::new("B003.py"))] #[test_case(Rule::CachedInstanceMethod, Path::new("B019.py"))] + #[test_case(Rule::ClassAsDataStructure, Path::new("class_as_data_structure.py"))] #[test_case(Rule::DuplicateHandlerException, Path::new("B014.py"))] #[test_case(Rule::DuplicateTryBlockException, Path::new("B025.py"))] #[test_case(Rule::DuplicateValue, Path::new("B033.py"))] diff --git a/crates/ruff_linter/src/rules/pylint/rules/class_as_data_structure.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs similarity index 99% rename from crates/ruff_linter/src/rules/pylint/rules/class_as_data_structure.rs rename to crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs index 17add9cb8c87e..7e61c32eb1b67 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/class_as_data_structure.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs @@ -42,7 +42,7 @@ impl Violation for ClassAsDataStructure { } } -/// R0903 +/// B903 pub(crate) fn class_as_data_structure(checker: &mut Checker, class_def: &ast::StmtClassDef) { // allow decorated classes if !class_def.decorator_list.is_empty() { diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/mod.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/mod.rs index 8fcca6883ee3f..dcf29d0fc1552 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/mod.rs @@ -4,6 +4,7 @@ pub(crate) use assert_raises_exception::*; pub(crate) use assignment_to_os_environ::*; pub(crate) use batched_without_explicit_strict::*; pub(crate) use cached_instance_method::*; +pub(crate) use class_as_data_structure::*; pub(crate) use duplicate_exceptions::*; pub(crate) use duplicate_value::*; pub(crate) use except_with_empty_tuple::*; @@ -43,6 +44,7 @@ mod assert_raises_exception; mod assignment_to_os_environ; mod batched_without_explicit_strict; mod cached_instance_method; +mod class_as_data_structure; mod duplicate_exceptions; mod duplicate_value; mod except_with_empty_tuple; diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__BB903_class_as_data_structure.py.snap b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__BB903_class_as_data_structure.py.snap new file mode 100644 index 0000000000000..6b92e1161505f --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__BB903_class_as_data_structure.py.snap @@ -0,0 +1,89 @@ +--- +source: crates/ruff_linter/src/rules/flake8_bugbear/mod.rs +--- +class_as_data_structure.py:6:1: BB903 Class could be dataclass or namedtuple + | +6 | / class Point: # PLR0903 +7 | | def __init__(self, x: float, y: float) -> None: +8 | | self.x = x +9 | | self.y = y + | |__________________^ BB903 + | + +class_as_data_structure.py:40:1: BB903 Class could be dataclass or namedtuple + | +38 | ... +39 | +40 | / class C: +41 | | c: int +42 | | def __init__(self,d:list):... + | |_________________________________^ BB903 +43 | +44 | class D: + | + +class_as_data_structure.py:44:1: BB903 Class could be dataclass or namedtuple + | +42 | def __init__(self,d:list):... +43 | +44 | / class D: +45 | | """This class has a docstring.""" +46 | | # this next method is an init +47 | | def __init__(self,e:dict):... + | |_________________________________^ BB903 +48 | +49 | # <--- begin flake8-bugbear tests below + | + +class_as_data_structure.py:61:1: BB903 Class could be dataclass or namedtuple + | +61 | / class NoWarningsClassAttributes: +62 | | spam = "ham" +63 | | +64 | | def __init__(self, foo:int, bar:list): +65 | | self.foo = foo +66 | | self.bar = bar + | |______________________^ BB903 + | + +class_as_data_structure.py:69:1: BB903 Class could be dataclass or namedtuple + | +69 | / class NoWarningsComplicatedAssignment: +70 | | def __init__(self, foo:int, bar:list): +71 | | self.foo = foo +72 | | self.bar = bar +73 | | self.spam = " - ".join([foo, bar]) + | |__________________________________________^ BB903 + | + +class_as_data_structure.py:76:1: BB903 Class could be dataclass or namedtuple + | +76 | / class NoWarningsMoreStatements: +77 | | def __init__(self, foo:int, bar:list): +78 | | foo = " - ".join([foo, bar]) +79 | | self.foo = foo +80 | | self.bar = bar + | |______________________^ BB903 + | + +class_as_data_structure.py:83:1: BB903 Class could be dataclass or namedtuple + | +83 | / class Warnings: +84 | | def __init__(self, foo:int, bar:list): +85 | | self.foo = foo +86 | | self.bar = bar + | |______________________^ BB903 + | + +class_as_data_structure.py:89:1: BB903 Class could be dataclass or namedtuple + | +89 | / class WarningsWithDocstring: +90 | | """A docstring should not be an impediment to a warning""" +91 | | +92 | | def __init__(self, foo:int, bar:list): +93 | | self.foo = foo +94 | | self.bar = bar + | |______________________^ BB903 +95 | +96 | # <-- end flake8-bugbear tests + | diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 939b512a10ee5..784f3f89954a9 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -207,7 +207,6 @@ mod tests { #[test_case(Rule::TooManyNestedBlocks, Path::new("too_many_nested_blocks.py"))] #[test_case(Rule::DictIndexMissingItems, Path::new("dict_index_missing_items.py"))] #[test_case(Rule::DictIterMissingItems, Path::new("dict_iter_missing_items.py"))] - #[test_case(Rule::ClassAsDataStructure, Path::new("too_few_public_methods.py"))] #[test_case( Rule::UnnecessaryDictIndexLookup, Path::new("unnecessary_dict_index_lookup.py") diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 14a9da98da16f..8d019d0887a48 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -10,7 +10,6 @@ pub(crate) use bad_string_format_type::*; pub(crate) use bidirectional_unicode::*; pub(crate) use binary_op_exception::*; pub(crate) use boolean_chained_comparison::*; -pub(crate) use class_as_data_structure::*; pub(crate) use collapsible_else_if::*; pub(crate) use compare_to_empty_string::*; pub(crate) use comparison_of_constant::*; @@ -118,7 +117,6 @@ mod bad_string_format_type; mod bidirectional_unicode; mod binary_op_exception; mod boolean_chained_comparison; -mod class_as_data_structure; mod collapsible_else_if; mod compare_to_empty_string; mod comparison_of_constant; diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0903_too_few_public_methods.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0903_too_few_public_methods.py.snap deleted file mode 100644 index adeeaaa9cb5ea..0000000000000 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0903_too_few_public_methods.py.snap +++ /dev/null @@ -1,34 +0,0 @@ ---- -source: crates/ruff_linter/src/rules/pylint/mod.rs ---- -too_few_public_methods.py:6:1: PLR0903 Class could be dataclass or namedtuple - | -6 | / class Point: # PLR0903 -7 | | def __init__(self, x: float, y: float) -> None: -8 | | self.x = x -9 | | self.y = y - | |__________________^ PLR0903 - | - -too_few_public_methods.py:40:1: PLR0903 Class could be dataclass or namedtuple - | -38 | ... -39 | -40 | / class C: -41 | | c: int -42 | | def __init__(self,d:list):... - | |_________________________________^ PLR0903 -43 | -44 | class D: - | - -too_few_public_methods.py:44:1: PLR0903 Class could be dataclass or namedtuple - | -42 | def __init__(self,d:list):... -43 | -44 | / class D: -45 | | """This class has a docstring.""" -46 | | # this next method is an init -47 | | def __init__(self,e:dict):... - | |_________________________________^ PLR0903 - | diff --git a/ruff.schema.json b/ruff.schema.json index b8125949f86c7..b027979b421b3 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2911,6 +2911,10 @@ "B909", "B91", "B911", + "BB", + "BB9", + "BB90", + "BB903", "BLE", "BLE0", "BLE00", @@ -3559,7 +3563,6 @@ "PLR0402", "PLR09", "PLR090", - "PLR0903", "PLR0904", "PLR091", "PLR0911", From fa2b724f4d60e597cfd90800b17921ba0010081c Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Fri, 3 Jan 2025 16:58:19 -0600 Subject: [PATCH 13/18] inits must have only simple assignments --- .../rules/class_as_data_structure.rs | 26 ++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs index 7e61c32eb1b67..024d0b502f7d8 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs @@ -1,6 +1,6 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; -use ruff_python_ast as ast; +use ruff_python_ast::{self as ast}; use ruff_python_semantic::analyze::visibility::{self, Visibility::Public}; use ruff_text_size::Ranged; @@ -72,6 +72,12 @@ pub(crate) fn class_as_data_structure(checker: &mut Checker, class_def: &ast::St // skip `self` .skip(1) .all(|param| param.annotation().is_some() && !param.is_variadic()) + // `__init__` should not have complicated logic in it + // only assignments + && func_def + .body + .iter() + .all(is_simple_assignment_to_attribute) { has_dunder_init = true; } @@ -93,3 +99,21 @@ pub(crate) fn class_as_data_structure(checker: &mut Checker, class_def: &ast::St .push(Diagnostic::new(ClassAsDataStructure, class_def.range())); } } + +// Checks whether a statement +// is a, possibly augmented, +// assignment of a name to an attribute. +fn is_simple_assignment_to_attribute(stmt: &ast::Stmt) -> bool { + match stmt { + ast::Stmt::Assign(ast::StmtAssign { targets, value, .. }) => { + let [target] = targets.as_slice() else { + return false; + }; + target.is_attribute_expr() & value.is_name_expr() + } + ast::Stmt::AnnAssign(ast::StmtAnnAssign { target, value, .. }) => { + target.is_attribute_expr() & value.as_ref().is_some_and(|val| val.is_name_expr()) + } + _ => false, + } +} From f088edaa96ef120c62cccbcc20b43ffe92468999 Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Fri, 3 Jan 2025 16:58:46 -0600 Subject: [PATCH 14/18] remove extra B in code and update tests --- .../flake8_bugbear/class_as_data_structure.py | 14 +-- crates/ruff_linter/src/codes.rs | 2 +- ...ests__B903_class_as_data_structure.py.snap | 71 +++++++++++++++ ...sts__BB903_class_as_data_structure.py.snap | 89 ------------------- ruff.schema.json | 5 +- 5 files changed, 81 insertions(+), 100 deletions(-) create mode 100644 crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B903_class_as_data_structure.py.snap delete mode 100644 crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__BB903_class_as_data_structure.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/class_as_data_structure.py b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/class_as_data_structure.py index 67d8f1c4f892e..fadc6af24c94a 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/class_as_data_structure.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/class_as_data_structure.py @@ -3,7 +3,7 @@ from dataclasses import dataclass -class Point: # PLR0903 +class Point: # B903 def __init__(self, x: float, y: float) -> None: self.x = x self.y = y @@ -30,21 +30,23 @@ class CustomException(Exception): # OK ... -class A: +class A: # OK class B: ... def __init__(self): ... -class C: +class C: # B903 c: int - def __init__(self,d:list):... + def __init__(self,d:list): + self.d = d -class D: +class D: # B903 """This class has a docstring.""" # this next method is an init - def __init__(self,e:dict):... + def __init__(self,e:dict): + self.e = e # <--- begin flake8-bugbear tests below # (we have modified them to have type annotations, diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index ba866e30e674f..44e5a966b3527 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -356,7 +356,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8Bugbear, "035") => (RuleGroup::Stable, rules::flake8_bugbear::rules::StaticKeyDictComprehension), (Flake8Bugbear, "039") => (RuleGroup::Stable, rules::flake8_bugbear::rules::MutableContextvarDefault), (Flake8Bugbear, "901") => (RuleGroup::Preview, rules::flake8_bugbear::rules::ReturnInGenerator), - (Flake8Bugbear, "B903") => (RuleGroup::Preview, rules::flake8_bugbear::rules::ClassAsDataStructure), + (Flake8Bugbear, "903") => (RuleGroup::Preview, rules::flake8_bugbear::rules::ClassAsDataStructure), (Flake8Bugbear, "904") => (RuleGroup::Stable, rules::flake8_bugbear::rules::RaiseWithoutFromInsideExcept), (Flake8Bugbear, "905") => (RuleGroup::Stable, rules::flake8_bugbear::rules::ZipWithoutExplicitStrict), (Flake8Bugbear, "909") => (RuleGroup::Preview, rules::flake8_bugbear::rules::LoopIteratorMutation), diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B903_class_as_data_structure.py.snap b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B903_class_as_data_structure.py.snap new file mode 100644 index 0000000000000..eea9b90dd5bfe --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B903_class_as_data_structure.py.snap @@ -0,0 +1,71 @@ +--- +source: crates/ruff_linter/src/rules/flake8_bugbear/mod.rs +--- +class_as_data_structure.py:6:1: B903 Class could be dataclass or namedtuple + | +6 | / class Point: # B903 +7 | | def __init__(self, x: float, y: float) -> None: +8 | | self.x = x +9 | | self.y = y + | |__________________^ B903 + | + +class_as_data_structure.py:40:1: B903 Class could be dataclass or namedtuple + | +38 | ... +39 | +40 | / class C: # B903 +41 | | c: int +42 | | def __init__(self,d:list): +43 | | self.d = d + | |__________________^ B903 +44 | +45 | class D: # B903 + | + +class_as_data_structure.py:45:1: B903 Class could be dataclass or namedtuple + | +43 | self.d = d +44 | +45 | / class D: # B903 +46 | | """This class has a docstring.""" +47 | | # this next method is an init +48 | | def __init__(self,e:dict): +49 | | self.e = e + | |__________________^ B903 +50 | +51 | # <--- begin flake8-bugbear tests below + | + +class_as_data_structure.py:63:1: B903 Class could be dataclass or namedtuple + | +63 | / class NoWarningsClassAttributes: +64 | | spam = "ham" +65 | | +66 | | def __init__(self, foo:int, bar:list): +67 | | self.foo = foo +68 | | self.bar = bar + | |______________________^ B903 + | + +class_as_data_structure.py:85:1: B903 Class could be dataclass or namedtuple + | +85 | / class Warnings: +86 | | def __init__(self, foo:int, bar:list): +87 | | self.foo = foo +88 | | self.bar = bar + | |______________________^ B903 + | + +class_as_data_structure.py:91:1: B903 Class could be dataclass or namedtuple + | +91 | / class WarningsWithDocstring: +92 | | """A docstring should not be an impediment to a warning""" +93 | | +94 | | def __init__(self, foo:int, bar:list): +95 | | self.foo = foo +96 | | self.bar = bar + | |______________________^ B903 +97 | +98 | # <-- end flake8-bugbear tests + | diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__BB903_class_as_data_structure.py.snap b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__BB903_class_as_data_structure.py.snap deleted file mode 100644 index 6b92e1161505f..0000000000000 --- a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__BB903_class_as_data_structure.py.snap +++ /dev/null @@ -1,89 +0,0 @@ ---- -source: crates/ruff_linter/src/rules/flake8_bugbear/mod.rs ---- -class_as_data_structure.py:6:1: BB903 Class could be dataclass or namedtuple - | -6 | / class Point: # PLR0903 -7 | | def __init__(self, x: float, y: float) -> None: -8 | | self.x = x -9 | | self.y = y - | |__________________^ BB903 - | - -class_as_data_structure.py:40:1: BB903 Class could be dataclass or namedtuple - | -38 | ... -39 | -40 | / class C: -41 | | c: int -42 | | def __init__(self,d:list):... - | |_________________________________^ BB903 -43 | -44 | class D: - | - -class_as_data_structure.py:44:1: BB903 Class could be dataclass or namedtuple - | -42 | def __init__(self,d:list):... -43 | -44 | / class D: -45 | | """This class has a docstring.""" -46 | | # this next method is an init -47 | | def __init__(self,e:dict):... - | |_________________________________^ BB903 -48 | -49 | # <--- begin flake8-bugbear tests below - | - -class_as_data_structure.py:61:1: BB903 Class could be dataclass or namedtuple - | -61 | / class NoWarningsClassAttributes: -62 | | spam = "ham" -63 | | -64 | | def __init__(self, foo:int, bar:list): -65 | | self.foo = foo -66 | | self.bar = bar - | |______________________^ BB903 - | - -class_as_data_structure.py:69:1: BB903 Class could be dataclass or namedtuple - | -69 | / class NoWarningsComplicatedAssignment: -70 | | def __init__(self, foo:int, bar:list): -71 | | self.foo = foo -72 | | self.bar = bar -73 | | self.spam = " - ".join([foo, bar]) - | |__________________________________________^ BB903 - | - -class_as_data_structure.py:76:1: BB903 Class could be dataclass or namedtuple - | -76 | / class NoWarningsMoreStatements: -77 | | def __init__(self, foo:int, bar:list): -78 | | foo = " - ".join([foo, bar]) -79 | | self.foo = foo -80 | | self.bar = bar - | |______________________^ BB903 - | - -class_as_data_structure.py:83:1: BB903 Class could be dataclass or namedtuple - | -83 | / class Warnings: -84 | | def __init__(self, foo:int, bar:list): -85 | | self.foo = foo -86 | | self.bar = bar - | |______________________^ BB903 - | - -class_as_data_structure.py:89:1: BB903 Class could be dataclass or namedtuple - | -89 | / class WarningsWithDocstring: -90 | | """A docstring should not be an impediment to a warning""" -91 | | -92 | | def __init__(self, foo:int, bar:list): -93 | | self.foo = foo -94 | | self.bar = bar - | |______________________^ BB903 -95 | -96 | # <-- end flake8-bugbear tests - | diff --git a/ruff.schema.json b/ruff.schema.json index b027979b421b3..0e3330bad58c4 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2906,15 +2906,12 @@ "B9", "B90", "B901", + "B903", "B904", "B905", "B909", "B91", "B911", - "BB", - "BB9", - "BB90", - "BB903", "BLE", "BLE0", "BLE00", From 6b95bc1e44f8d70dbcc4cf54f837f2ad4fda5cdb Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Mon, 6 Jan 2025 20:58:45 -0600 Subject: [PATCH 15/18] doc comment nits --- .../rules/flake8_bugbear/rules/class_as_data_structure.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs index 024d0b502f7d8..f34c3057569ef 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs @@ -8,7 +8,7 @@ use crate::checkers::ast::Checker; /// ## What it does /// Checks for classes that only have a public `__init__` method, -/// without base classes and 0 decorators. +/// without base classes and decorators. /// /// ## Why is this bad? /// Classes with just an `__init__` are possibly better off @@ -100,8 +100,7 @@ pub(crate) fn class_as_data_structure(checker: &mut Checker, class_def: &ast::St } } -// Checks whether a statement -// is a, possibly augmented, +// Checks whether a statement is a, possibly augmented, // assignment of a name to an attribute. fn is_simple_assignment_to_attribute(stmt: &ast::Stmt) -> bool { match stmt { From c128467276b71dba8ecae12f6d1ab6c4863a2156 Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Mon, 6 Jan 2025 20:59:04 -0600 Subject: [PATCH 16/18] skip stub files --- .../rules/flake8_bugbear/rules/class_as_data_structure.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs index f34c3057569ef..2ff399ca9abf8 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs @@ -44,6 +44,11 @@ impl Violation for ClassAsDataStructure { /// B903 pub(crate) fn class_as_data_structure(checker: &mut Checker, class_def: &ast::StmtClassDef) { + // skip stub files + if checker.source_type.is_stub() { + return; + } + // allow decorated classes if !class_def.decorator_list.is_empty() { return; From 524162c6502e3296fc8a3b3b2067b9579ca1867d Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Mon, 6 Jan 2025 21:01:31 -0600 Subject: [PATCH 17/18] skip lint if strange statement in class body --- .../flake8_bugbear/rules/class_as_data_structure.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs index 2ff399ca9abf8..c903a841a0569 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs @@ -90,11 +90,15 @@ pub(crate) fn class_as_data_structure(checker: &mut Checker, class_def: &ast::St public_methods += 1; } } - ast::Stmt::ClassDef(_) => { - // allow classes with nested classes, often used for config + // Ignore class variables + ast::Stmt::Assign(_) | ast::Stmt::AnnAssign(_) | + // and expressions (e.g. string literals) + ast::Stmt::Expr(_) => {} + _ => { + // Bail for anything else - e.g. nested classes + // or conditional methods. return; } - _ => {} } } From 6ece0f79910945adf17a6281272afd5db9038f14 Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Mon, 6 Jan 2025 21:01:45 -0600 Subject: [PATCH 18/18] use lazy boolean --- .../src/rules/flake8_bugbear/rules/class_as_data_structure.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs index c903a841a0569..035ab4bcbcc83 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/class_as_data_structure.rs @@ -117,10 +117,10 @@ fn is_simple_assignment_to_attribute(stmt: &ast::Stmt) -> bool { let [target] = targets.as_slice() else { return false; }; - target.is_attribute_expr() & value.is_name_expr() + target.is_attribute_expr() && value.is_name_expr() } ast::Stmt::AnnAssign(ast::StmtAnnAssign { target, value, .. }) => { - target.is_attribute_expr() & value.as_ref().is_some_and(|val| val.is_name_expr()) + target.is_attribute_expr() && value.as_ref().is_some_and(|val| val.is_name_expr()) } _ => false, }