From 372e369b3c9b30a0f18b701e03b4cc893204401e Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 4 Dec 2023 12:53:30 -0500 Subject: [PATCH] Update fixes; change default to five --- crates/ruff_linter/src/codes.rs | 2 +- crates/ruff_linter/src/rules/pylint/mod.rs | 4 +- .../rules/pylint/rules/too_many_positional.rs | 42 ++++++++++++------- .../ruff_linter/src/rules/pylint/settings.rs | 4 +- ...tests__PLR0917_too_many_positional.py.snap | 25 +++++++++++ ...s__pylint__tests__max_positional_args.snap | 22 ++++++++++ crates/ruff_workspace/src/options.rs | 6 ++- ruff.schema.json | 10 +++++ 8 files changed, 93 insertions(+), 22 deletions(-) create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0917_too_many_positional.py.snap create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__max_positional_args.snap diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index e8ba90dc1dbcb..fd0b85727bbec 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -254,7 +254,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "R0913") => (RuleGroup::Stable, rules::pylint::rules::TooManyArguments), (Pylint, "R0915") => (RuleGroup::Stable, rules::pylint::rules::TooManyStatements), (Pylint, "R0916") => (RuleGroup::Preview, rules::pylint::rules::TooManyBooleanExpressions), - (Pylint, "R0917") => (RuleGroup::Stable, rules::pylint::rules::TooManyPositional), + (Pylint, "R0917") => (RuleGroup::Preview, rules::pylint::rules::TooManyPositional), (Pylint, "R1701") => (RuleGroup::Stable, rules::pylint::rules::RepeatedIsinstanceCalls), (Pylint, "R1704") => (RuleGroup::Preview, rules::pylint::rules::RedefinedArgumentFromLocal), (Pylint, "R1711") => (RuleGroup::Stable, rules::pylint::rules::UselessReturn), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index ffefb3d460bc0..3933e07bf9e02 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -251,12 +251,12 @@ mod tests { } #[test] - fn max_pos_args() -> Result<()> { + fn max_positional_args() -> Result<()> { let diagnostics = test_path( Path::new("pylint/too_many_positional_params.py"), &LinterSettings { pylint: pylint::settings::Settings { - max_pos_args: 4, + max_positional_args: 4, ..pylint::settings::Settings::default() }, ..LinterSettings::for_rule(Rule::TooManyPositional) diff --git a/crates/ruff_linter/src/rules/pylint/rules/too_many_positional.rs b/crates/ruff_linter/src/rules/pylint/rules/too_many_positional.rs index 42fea846a2a13..369b6c63bc6e9 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/too_many_positional.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/too_many_positional.rs @@ -1,33 +1,45 @@ -use ruff_diagnostics::{Violation, Diagnostic}; +use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::{Parameters, Stmt, identifier::Identifier}; +use ruff_python_ast::{identifier::Identifier, Parameters, Stmt}; use crate::checkers::ast::Checker; /// ## What it does /// Checks for function definitions that include too many positional arguments. /// +/// By default, this rule allows up to five arguments, as configured by the +/// [`pylint.max-positional-args`] option. +/// /// ## Why is this bad? -/// The position of arguments that are not defined as keyword-only -/// becomes part of the function’s API. This encourages consumers of the API -/// to specify arguments positionally, making their code harder to read, -/// and complicating refactoring of the API, such as deprecating a parameter. +/// Functions with many arguments are harder to understand, maintain, and call. +/// This is especially true for functions with many positional arguments, as +/// providing arguments positionally is more error-prone and less clear to +/// readers than providing arguments by name. +/// +/// Consider refactoring functions with many arguments into smaller functions +/// with fewer arguments, using objects to group related arguments, or +/// migrating to keyword-only arguments. /// /// ## Example /// ```python /// def plot(x, y, z, color, mark, add_trendline): /// ... -/// -/// plot(1, 2, 3, 'r', '*', True) +/// +/// +/// plot(1, 2, 3, "r", "*", True) /// ``` /// /// Use instead: /// ```python /// def plot(x, y, z, *, color, mark, add_trendline): /// ... -/// -/// plot(1, 2, 3, color='r', mark='*', add_trendline=True) +/// +/// +/// plot(1, 2, 3, color="r", mark="*", add_trendline=True) /// ``` +/// +/// ## Options +/// - `pylint.max-positional-args` #[violation] pub struct TooManyPositional { c_pos: usize, @@ -42,9 +54,9 @@ impl Violation for TooManyPositional { } } -/// PLRR0917 +/// PLR0917 pub(crate) fn too_many_positional(checker: &mut Checker, parameters: &Parameters, stmt: &Stmt) { - let num_positional = parameters + let num_positional_args = parameters .args .iter() .chain(¶meters.posonlyargs) @@ -55,11 +67,11 @@ pub(crate) fn too_many_positional(checker: &mut Checker, parameters: &Parameters .is_match(&arg.parameter.name) }) .count(); - if num_positional > checker.settings.pylint.max_pos_args { + if num_positional_args > checker.settings.pylint.max_positional_args { checker.diagnostics.push(Diagnostic::new( TooManyPositional { - c_pos: num_positional, - max_pos: checker.settings.pylint.max_pos_args, + c_pos: num_positional_args, + max_pos: checker.settings.pylint.max_positional_args, }, stmt.identifier(), )); diff --git a/crates/ruff_linter/src/rules/pylint/settings.rs b/crates/ruff_linter/src/rules/pylint/settings.rs index 564f7bea7f5cc..020390704c7f8 100644 --- a/crates/ruff_linter/src/rules/pylint/settings.rs +++ b/crates/ruff_linter/src/rules/pylint/settings.rs @@ -39,7 +39,7 @@ pub struct Settings { pub allow_magic_value_types: Vec, pub allow_dunder_method_names: FxHashSet, pub max_args: usize, - pub max_pos_args: usize, + pub max_positional_args: usize, pub max_returns: usize, pub max_bool_expr: usize, pub max_branches: usize, @@ -53,7 +53,7 @@ impl Default for Settings { allow_magic_value_types: vec![ConstantType::Str, ConstantType::Bytes], allow_dunder_method_names: FxHashSet::default(), max_args: 5, - max_pos_args: 3, + max_positional_args: 5, max_returns: 6, max_bool_expr: 5, max_branches: 12, diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0917_too_many_positional.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0917_too_many_positional.py.snap new file mode 100644 index 0000000000000..4578419f43545 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0917_too_many_positional.py.snap @@ -0,0 +1,25 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +too_many_positional.py:1:5: PLR0917 Too many positional arguments: (8/5) + | +1 | def f(x, y, z, t, u, v, w, r): # Too many positional arguments (8/3) + | ^ PLR0917 +2 | pass + | + +too_many_positional.py:21:5: PLR0917 Too many positional arguments: (6/5) + | +21 | def f(x, y, z, /, u, v, w): # Too many positional arguments (6/3) + | ^ PLR0917 +22 | pass + | + +too_many_positional.py:29:5: PLR0917 Too many positional arguments: (6/5) + | +29 | def f(x, y, z, a, b, c, *, u, v, w): # Too many positional arguments (6/3) + | ^ PLR0917 +30 | pass + | + + diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__max_positional_args.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__max_positional_args.snap new file mode 100644 index 0000000000000..98c964820770c --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__max_positional_args.snap @@ -0,0 +1,22 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +too_many_positional_params.py:3:5: PLR0917 Too many positional arguments: (7/4) + | +1 | # Too many positional arguments (7/4) for max_positional=4 +2 | # OK for dummy_variable_rgx ~ "skip_.*" +3 | def f(w, x, y, z, skip_t, skip_u, skip_v): + | ^ PLR0917 +4 | pass + | + +too_many_positional_params.py:9:5: PLR0917 Too many positional arguments: (7/4) + | + 7 | # Too many positional arguments (7/4) for max_args=4 + 8 | # Too many positional arguments (7/3) for dummy_variable_rgx ~ "skip_.*" + 9 | def f(w, x, y, z, t, u, v): + | ^ PLR0917 +10 | pass + | + + diff --git a/crates/ruff_workspace/src/options.rs b/crates/ruff_workspace/src/options.rs index a7ad3a16a6b78..f87a5223d9aed 100644 --- a/crates/ruff_workspace/src/options.rs +++ b/crates/ruff_workspace/src/options.rs @@ -2638,7 +2638,7 @@ pub struct PylintOptions { /// Maximum number of positional arguments allowed for a function or method definition /// (see: `PLR0917`). #[option(default = r"3", value_type = "int", example = r"max-pos-args = 3")] - pub max_pos_args: Option, + pub max_positional_args: Option, /// Maximum number of statements allowed for a function or method body (see: /// `PLR0915`). @@ -2668,7 +2668,9 @@ impl PylintOptions { .unwrap_or(defaults.allow_magic_value_types), allow_dunder_method_names: self.allow_dunder_method_names.unwrap_or_default(), max_args: self.max_args.unwrap_or(defaults.max_args), - max_pos_args: self.max_pos_args.unwrap_or(defaults.max_pos_args), + max_positional_args: self + .max_positional_args + .unwrap_or(defaults.max_positional_args), max_bool_expr: self.max_bool_expr.unwrap_or(defaults.max_bool_expr), max_returns: self.max_returns.unwrap_or(defaults.max_returns), max_branches: self.max_branches.unwrap_or(defaults.max_branches), diff --git a/ruff.schema.json b/ruff.schema.json index b0584cad84735..d962edc15e141 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2369,6 +2369,15 @@ "format": "uint", "minimum": 0.0 }, + "max-positional-args": { + "description": "Maximum number of positional arguments allowed for a function or method definition (see: `PLR0917`).", + "type": [ + "integer", + "null" + ], + "format": "uint", + "minimum": 0.0 + }, "max-public-methods": { "description": "Maximum number of public methods allowed for a class (see: `PLR0904`).", "type": [ @@ -3117,6 +3126,7 @@ "PLR0913", "PLR0915", "PLR0916", + "PLR0917", "PLR1", "PLR17", "PLR170",