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

[pylint] Implement too-many-public-methods rule (PLR0904) #6179

Merged
merged 4 commits into from
Sep 14, 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
@@ -0,0 +1,60 @@
class Everything:
foo = 1

def __init__(self):
pass

def _private(self):
pass

def method1(self):
pass

def method2(self):
pass

def method3(self):
pass

def method4(self):
pass

def method5(self):
pass

def method6(self):
pass

def method7(self):
pass

def method8(self):
pass

def method9(self):
pass

class Small:
def __init__(self):
pass

def _private(self):
pass

def method1(self):
pass

def method2(self):
pass

def method3(self):
pass

def method4(self):
pass

def method5(self):
pass

def method6(self):
pass
7 changes: 7 additions & 0 deletions crates/ruff/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,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::TooManyPublicMethods) {
pylint::rules::too_many_public_methods(
checker,
class_def,
checker.settings.pylint.max_public_methods,
);
}
if checker.enabled(Rule::GlobalStatement) {
pylint::rules::global_statement(checker, name);
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Pylint, "W1510") => (RuleGroup::Unspecified, rules::pylint::rules::SubprocessRunWithoutCheck),
#[allow(deprecated)]
(Pylint, "W1641") => (RuleGroup::Nursery, rules::pylint::rules::EqWithoutHash),
(Pylint, "R0904") => (RuleGroup::Preview, rules::pylint::rules::TooManyPublicMethods),
(Pylint, "W2901") => (RuleGroup::Unspecified, rules::pylint::rules::RedefinedLoopName),
#[allow(deprecated)]
(Pylint, "W3201") => (RuleGroup::Nursery, rules::pylint::rules::BadDunderMethodName),
Expand Down
16 changes: 16 additions & 0 deletions crates/ruff/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -256,4 +256,20 @@ mod tests {
assert_messages!(diagnostics);
Ok(())
}

#[test]
fn too_many_public_methods() -> Result<()> {
let diagnostics = test_path(
Path::new("pylint/too_many_public_methods.py"),
&Settings {
pylint: pylint::settings::Settings {
max_public_methods: 7,
..pylint::settings::Settings::default()
},
..Settings::for_rules(vec![Rule::TooManyPublicMethods])
},
)?;
assert_messages!(diagnostics);
Ok(())
}
}
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 @@ -43,6 +43,7 @@ pub(crate) use subprocess_run_without_check::*;
pub(crate) use sys_exit_alias::*;
pub(crate) use too_many_arguments::*;
pub(crate) use too_many_branches::*;
pub(crate) use too_many_public_methods::*;
pub(crate) use too_many_return_statements::*;
pub(crate) use too_many_statements::*;
pub(crate) use type_bivariance::*;
Expand Down Expand Up @@ -101,6 +102,7 @@ mod subprocess_run_without_check;
mod sys_exit_alias;
mod too_many_arguments;
mod too_many_branches;
mod too_many_public_methods;
mod too_many_return_statements;
mod too_many_statements;
mod type_bivariance;
Expand Down
126 changes: 126 additions & 0 deletions crates/ruff/src/rules/pylint/rules/too_many_public_methods.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
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 many public methods
///
/// By default, this rule allows up to 20 statements, as configured by the
/// [`pylint.max-public-methods`] option.
///
/// ## Why is this bad?
/// Classes with many public methods are harder to understand
/// and maintain.
///
/// Instead, consider refactoring the class into separate classes.
///
/// ## Example
/// Assuming that `pylint.max-public-settings` is set to 5:
/// ```python
/// class Linter:
/// def __init__(self):
/// pass
///
/// def pylint(self):
/// pass
///
/// def pylint_settings(self):
/// pass
///
/// def flake8(self):
/// pass
///
/// def flake8_settings(self):
/// pass
///
/// def pydocstyle(self):
/// pass
///
/// def pydocstyle_settings(self):
/// pass
/// ```
///
/// Use instead:
/// ```python
/// class Linter:
/// def __init__(self):
/// self.pylint = Pylint()
/// self.flake8 = Flake8()
/// self.pydocstyle = Pydocstyle()
///
/// def lint(self):
/// pass
///
///
/// class Pylint:
/// def lint(self):
/// pass
///
/// def settings(self):
/// pass
///
///
/// class Flake8:
/// def lint(self):
/// pass
///
/// def settings(self):
/// pass
///
///
/// class Pydocstyle:
/// def lint(self):
/// pass
///
/// def settings(self):
/// pass
/// ```
///
/// ## Options
/// - `pylint.max-public-methods`
#[violation]
pub struct TooManyPublicMethods {
methods: usize,
max_methods: usize,
}

impl Violation for TooManyPublicMethods {
#[derive_message_formats]
fn message(&self) -> String {
let TooManyPublicMethods {
methods,
max_methods,
} = self;
format!("Too many public methods ({methods} > {max_methods})")
}
}

/// R0904
pub(crate) fn too_many_public_methods(
checker: &mut Checker,
class_def: &ast::StmtClassDef,
max_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 > max_methods {
checker.diagnostics.push(Diagnostic::new(
TooManyPublicMethods {
methods,
max_methods,
},
class_def.range(),
));
}
}
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/pylint/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ pub struct Settings {
pub max_returns: usize,
pub max_branches: usize,
pub max_statements: usize,
pub max_public_methods: usize,
}

impl Default for Settings {
Expand All @@ -52,6 +53,7 @@ impl Default for Settings {
max_returns: 6,
max_branches: 12,
max_statements: 50,
max_public_methods: 20,
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
---
source: crates/ruff/src/rules/pylint/mod.rs
---
too_many_public_methods.py:1:1: PLR0904 Too many public methods (10 > 7)
|
1 | / class Everything:
2 | | foo = 1
3 | |
4 | | def __init__(self):
5 | | pass
6 | |
7 | | def _private(self):
8 | | pass
9 | |
10 | | def method1(self):
11 | | pass
12 | |
13 | | def method2(self):
14 | | pass
15 | |
16 | | def method3(self):
17 | | pass
18 | |
19 | | def method4(self):
20 | | pass
21 | |
22 | | def method5(self):
23 | | pass
24 | |
25 | | def method6(self):
26 | | pass
27 | |
28 | | def method7(self):
29 | | pass
30 | |
31 | | def method8(self):
32 | | pass
33 | |
34 | | def method9(self):
35 | | pass
| |____________^ PLR0904
36 |
37 | class Small:
|


2 changes: 1 addition & 1 deletion crates/ruff_python_semantic/src/analyze/visibility.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ pub(crate) fn function_visibility(function: &ast::StmtFunctionDef) -> Visibility
}
}

pub(crate) fn method_visibility(function: &ast::StmtFunctionDef) -> Visibility {
pub fn method_visibility(function: &ast::StmtFunctionDef) -> Visibility {
// Is this a setter or deleter?
if function.decorator_list.iter().any(|decorator| {
collect_call_path(&decorator.expression).is_some_and(|call_path| {
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_workspace/src/configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -851,7 +851,7 @@ mod tests {
Rule::QuadraticListSummation,
];

const PREVIEW_RULES: &[Rule] = &[Rule::SliceCopy];
const PREVIEW_RULES: &[Rule] = &[Rule::TooManyPublicMethods, Rule::SliceCopy];

#[allow(clippy::needless_pass_by_value)]
fn resolve_rules(
Expand Down
10 changes: 10 additions & 0 deletions crates/ruff_workspace/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2155,6 +2155,13 @@ pub struct PylintOptions {
/// Maximum number of statements allowed for a function or method body (see:
/// `PLR0915`).
pub max_statements: Option<usize>,
#[option(
default = r"20",
value_type = "int",
example = r"max-public-methods = 20"
)]
/// Maximum number of public methods allowed for a class (see: `PLR0904`).
pub max_public_methods: Option<usize>,
}

impl PylintOptions {
Expand All @@ -2168,6 +2175,9 @@ impl PylintOptions {
max_returns: self.max_returns.unwrap_or(defaults.max_returns),
max_branches: self.max_branches.unwrap_or(defaults.max_branches),
max_statements: self.max_statements.unwrap_or(defaults.max_statements),
max_public_methods: self
.max_public_methods
.unwrap_or(defaults.max_public_methods),
}
}
}
Expand Down
11 changes: 11 additions & 0 deletions ruff.schema.json

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

Loading