mirror of https://github.com/astral-sh/ruff
[pylint] Implement `too-many-public-methods` rule (PLR0904) (#6179)
Implement https://pylint.pycqa.org/en/latest/user_guide/messages/refactor/too-many-public-methods.html Confusingly the rule page mentions a max of 7 while in practice it is 20. https://github.com/search?q=repo%3Apylint-dev%2Fpylint+max-public-methods&type=code ## Summary Implement pylint's R0904 ## Test Plan Unit tests.
This commit is contained in:
parent
36fa1fe359
commit
04183b0299
|
|
@ -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
|
||||||
|
|
@ -411,6 +411,13 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
|
||||||
if checker.enabled(Rule::EqWithoutHash) {
|
if checker.enabled(Rule::EqWithoutHash) {
|
||||||
pylint::rules::object_without_hash_method(checker, class_def);
|
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) {
|
if checker.enabled(Rule::GlobalStatement) {
|
||||||
pylint::rules::global_statement(checker, name);
|
pylint::rules::global_statement(checker, name);
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -269,6 +269,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
|
||||||
(Pylint, "W1510") => (RuleGroup::Unspecified, rules::pylint::rules::SubprocessRunWithoutCheck),
|
(Pylint, "W1510") => (RuleGroup::Unspecified, rules::pylint::rules::SubprocessRunWithoutCheck),
|
||||||
#[allow(deprecated)]
|
#[allow(deprecated)]
|
||||||
(Pylint, "W1641") => (RuleGroup::Nursery, rules::pylint::rules::EqWithoutHash),
|
(Pylint, "W1641") => (RuleGroup::Nursery, rules::pylint::rules::EqWithoutHash),
|
||||||
|
(Pylint, "R0904") => (RuleGroup::Preview, rules::pylint::rules::TooManyPublicMethods),
|
||||||
(Pylint, "W2901") => (RuleGroup::Unspecified, rules::pylint::rules::RedefinedLoopName),
|
(Pylint, "W2901") => (RuleGroup::Unspecified, rules::pylint::rules::RedefinedLoopName),
|
||||||
#[allow(deprecated)]
|
#[allow(deprecated)]
|
||||||
(Pylint, "W3201") => (RuleGroup::Nursery, rules::pylint::rules::BadDunderMethodName),
|
(Pylint, "W3201") => (RuleGroup::Nursery, rules::pylint::rules::BadDunderMethodName),
|
||||||
|
|
|
||||||
|
|
@ -256,4 +256,20 @@ mod tests {
|
||||||
assert_messages!(diagnostics);
|
assert_messages!(diagnostics);
|
||||||
Ok(())
|
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(())
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -43,6 +43,7 @@ pub(crate) use subprocess_run_without_check::*;
|
||||||
pub(crate) use sys_exit_alias::*;
|
pub(crate) use sys_exit_alias::*;
|
||||||
pub(crate) use too_many_arguments::*;
|
pub(crate) use too_many_arguments::*;
|
||||||
pub(crate) use too_many_branches::*;
|
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_return_statements::*;
|
||||||
pub(crate) use too_many_statements::*;
|
pub(crate) use too_many_statements::*;
|
||||||
pub(crate) use type_bivariance::*;
|
pub(crate) use type_bivariance::*;
|
||||||
|
|
@ -101,6 +102,7 @@ mod subprocess_run_without_check;
|
||||||
mod sys_exit_alias;
|
mod sys_exit_alias;
|
||||||
mod too_many_arguments;
|
mod too_many_arguments;
|
||||||
mod too_many_branches;
|
mod too_many_branches;
|
||||||
|
mod too_many_public_methods;
|
||||||
mod too_many_return_statements;
|
mod too_many_return_statements;
|
||||||
mod too_many_statements;
|
mod too_many_statements;
|
||||||
mod type_bivariance;
|
mod type_bivariance;
|
||||||
|
|
|
||||||
|
|
@ -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(),
|
||||||
|
));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
@ -42,6 +42,7 @@ pub struct Settings {
|
||||||
pub max_returns: usize,
|
pub max_returns: usize,
|
||||||
pub max_branches: usize,
|
pub max_branches: usize,
|
||||||
pub max_statements: usize,
|
pub max_statements: usize,
|
||||||
|
pub max_public_methods: usize,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl Default for Settings {
|
impl Default for Settings {
|
||||||
|
|
@ -52,6 +53,7 @@ impl Default for Settings {
|
||||||
max_returns: 6,
|
max_returns: 6,
|
||||||
max_branches: 12,
|
max_branches: 12,
|
||||||
max_statements: 50,
|
max_statements: 50,
|
||||||
|
max_public_methods: 20,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -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:
|
||||||
|
|
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -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?
|
// Is this a setter or deleter?
|
||||||
if function.decorator_list.iter().any(|decorator| {
|
if function.decorator_list.iter().any(|decorator| {
|
||||||
collect_call_path(&decorator.expression).is_some_and(|call_path| {
|
collect_call_path(&decorator.expression).is_some_and(|call_path| {
|
||||||
|
|
|
||||||
|
|
@ -851,7 +851,7 @@ mod tests {
|
||||||
Rule::QuadraticListSummation,
|
Rule::QuadraticListSummation,
|
||||||
];
|
];
|
||||||
|
|
||||||
const PREVIEW_RULES: &[Rule] = &[Rule::SliceCopy];
|
const PREVIEW_RULES: &[Rule] = &[Rule::TooManyPublicMethods, Rule::SliceCopy];
|
||||||
|
|
||||||
#[allow(clippy::needless_pass_by_value)]
|
#[allow(clippy::needless_pass_by_value)]
|
||||||
fn resolve_rules(
|
fn resolve_rules(
|
||||||
|
|
|
||||||
|
|
@ -2155,6 +2155,13 @@ pub struct PylintOptions {
|
||||||
/// Maximum number of statements allowed for a function or method body (see:
|
/// Maximum number of statements allowed for a function or method body (see:
|
||||||
/// `PLR0915`).
|
/// `PLR0915`).
|
||||||
pub max_statements: Option<usize>,
|
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 {
|
impl PylintOptions {
|
||||||
|
|
@ -2168,6 +2175,9 @@ impl PylintOptions {
|
||||||
max_returns: self.max_returns.unwrap_or(defaults.max_returns),
|
max_returns: self.max_returns.unwrap_or(defaults.max_returns),
|
||||||
max_branches: self.max_branches.unwrap_or(defaults.max_branches),
|
max_branches: self.max_branches.unwrap_or(defaults.max_branches),
|
||||||
max_statements: self.max_statements.unwrap_or(defaults.max_statements),
|
max_statements: self.max_statements.unwrap_or(defaults.max_statements),
|
||||||
|
max_public_methods: self
|
||||||
|
.max_public_methods
|
||||||
|
.unwrap_or(defaults.max_public_methods),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -1612,6 +1612,15 @@
|
||||||
"format": "uint",
|
"format": "uint",
|
||||||
"minimum": 0.0
|
"minimum": 0.0
|
||||||
},
|
},
|
||||||
|
"max-public-methods": {
|
||||||
|
"description": "Maximum number of public methods allowed for a class (see: `PLR0904`).",
|
||||||
|
"type": [
|
||||||
|
"integer",
|
||||||
|
"null"
|
||||||
|
],
|
||||||
|
"format": "uint",
|
||||||
|
"minimum": 0.0
|
||||||
|
},
|
||||||
"max-returns": {
|
"max-returns": {
|
||||||
"description": "Maximum number of return statements allowed for a function or method body (see `PLR0911`)",
|
"description": "Maximum number of return statements allowed for a function or method body (see `PLR0911`)",
|
||||||
"type": [
|
"type": [
|
||||||
|
|
@ -2296,6 +2305,8 @@
|
||||||
"PLR040",
|
"PLR040",
|
||||||
"PLR0402",
|
"PLR0402",
|
||||||
"PLR09",
|
"PLR09",
|
||||||
|
"PLR090",
|
||||||
|
"PLR0904",
|
||||||
"PLR091",
|
"PLR091",
|
||||||
"PLR0911",
|
"PLR0911",
|
||||||
"PLR0912",
|
"PLR0912",
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue