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-positional (PLR0917) #8995

Merged
merged 4 commits into from
Dec 4, 2023
Merged
Show file tree
Hide file tree
Changes from 2 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,30 @@
def f(x, y, z, t, u, v, w, r): # Too many positional arguments (8/3)
pass


def f(x): # OK
pass


def f(x, y, z, _t, _u, _v, _w, r): # OK (underscore-prefixed names are ignored
pass


def f(x, y, z, *, u=1, v=1, r=1): # OK
pass


def f(x=1, y=1, z=1): # OK
pass


def f(x, y, z, /, u, v, w): # Too many positional arguments (6/3)
pass


def f(x, y, z, *, u, v, w): # OK
pass


def f(x, y, z, a, b, c, *, u, v, w): # Too many positional arguments (6/3)
pass
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# Too many positional arguments (7/4) for max_positional=4
# OK for dummy_variable_rgx ~ "skip_.*"
def f(w, x, y, z, skip_t, skip_u, skip_v):
pass


# Too many positional arguments (7/4) for max_args=4
# Too many positional arguments (7/3) for dummy_variable_rgx ~ "skip_.*"
def f(w, x, y, z, t, u, v):
pass
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::TooManyArguments) {
pylint::rules::too_many_arguments(checker, parameters, stmt);
}
if checker.enabled(Rule::TooManyPositional) {
pylint::rules::too_many_positional(checker, parameters, stmt);
}
if checker.enabled(Rule::TooManyReturnStatements) {
if let Some(diagnostic) = pylint::rules::too_many_return_statements(
stmt,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +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, "R1701") => (RuleGroup::Stable, rules::pylint::rules::RepeatedIsinstanceCalls),
(Pylint, "R1704") => (RuleGroup::Preview, rules::pylint::rules::RedefinedArgumentFromLocal),
(Pylint, "R1711") => (RuleGroup::Stable, rules::pylint::rules::UselessReturn),
Expand Down
17 changes: 17 additions & 0 deletions crates/ruff_linter/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ mod tests {
#[test_case(Rule::RedefinedLoopName, Path::new("redefined_loop_name.py"))]
#[test_case(Rule::ReturnInInit, Path::new("return_in_init.py"))]
#[test_case(Rule::TooManyArguments, Path::new("too_many_arguments.py"))]
#[test_case(Rule::TooManyPositional, Path::new("too_many_positional.py"))]
#[test_case(Rule::TooManyBranches, Path::new("too_many_branches.py"))]
#[test_case(
Rule::TooManyReturnStatements,
Expand Down Expand Up @@ -249,6 +250,22 @@ mod tests {
Ok(())
}

#[test]
fn max_pos_args() -> Result<()> {
let diagnostics = test_path(
Path::new("pylint/too_many_positional_params.py"),
&LinterSettings {
pylint: pylint::settings::Settings {
max_pos_args: 4,
..pylint::settings::Settings::default()
},
..LinterSettings::for_rule(Rule::TooManyPositional)
},
)?;
assert_messages!(diagnostics);
Ok(())
}

#[test]
fn max_branches() -> Result<()> {
let diagnostics = test_path(
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ pub(crate) use sys_exit_alias::*;
pub(crate) use too_many_arguments::*;
pub(crate) use too_many_boolean_expressions::*;
pub(crate) use too_many_branches::*;
pub(crate) use too_many_positional::*;
pub(crate) use too_many_public_methods::*;
pub(crate) use too_many_return_statements::*;
pub(crate) use too_many_statements::*;
Expand Down Expand Up @@ -131,6 +132,7 @@ mod sys_exit_alias;
mod too_many_arguments;
mod too_many_boolean_expressions;
mod too_many_branches;
mod too_many_positional;
mod too_many_public_methods;
mod too_many_return_statements;
mod too_many_statements;
Expand Down
67 changes: 67 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/too_many_positional.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
use ruff_diagnostics::{Violation, Diagnostic};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{Parameters, Stmt, identifier::Identifier};

use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for function definitions that include too many positional arguments.
///
/// ## 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.
///
/// ## Example
/// ```python
/// def plot(x, y, z, color, mark, add_trendline):
/// ...
///
/// 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)
/// ```
#[violation]
pub struct TooManyPositional {
c_pos: usize,
max_pos: usize,
}

impl Violation for TooManyPositional {
#[derive_message_formats]
fn message(&self) -> String {
let TooManyPositional { c_pos, max_pos } = self;
format!("Too many positional arguments: ({c_pos}/{max_pos})")
}
}

/// PLRR0917
pub(crate) fn too_many_positional(checker: &mut Checker, parameters: &Parameters, stmt: &Stmt) {
let num_positional = parameters
.args
.iter()
.chain(&parameters.posonlyargs)
.filter(|arg| {
!checker
.settings
.dummy_variable_rgx
.is_match(&arg.parameter.name)
})
.count();
if num_positional > checker.settings.pylint.max_pos_args {
checker.diagnostics.push(Diagnostic::new(
TooManyPositional {
c_pos: num_positional,
max_pos: checker.settings.pylint.max_pos_args,
},
stmt.identifier(),
));
}
}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pylint/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ pub struct Settings {
pub allow_magic_value_types: Vec<ConstantType>,
pub allow_dunder_method_names: FxHashSet<String>,
pub max_args: usize,
pub max_pos_args: usize,
pub max_returns: usize,
pub max_bool_expr: usize,
pub max_branches: usize,
Expand All @@ -52,6 +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_returns: 6,
max_bool_expr: 5,
max_branches: 12,
Expand Down
6 changes: 6 additions & 0 deletions crates/ruff_workspace/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2635,6 +2635,11 @@ pub struct PylintOptions {
#[option(default = r"5", value_type = "int", example = r"max-args = 5")]
pub max_args: Option<usize>,

/// 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<usize>,

/// Maximum number of statements allowed for a function or method body (see:
/// `PLR0915`).
#[option(default = r"50", value_type = "int", example = r"max-statements = 50")]
Expand Down Expand Up @@ -2663,6 +2668,7 @@ 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_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),
Expand Down
Loading