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

Support rules from flake8-force-keyword-arguments #3269

Open
Riezebos opened this issue Feb 28, 2023 · 13 comments
Open

Support rules from flake8-force-keyword-arguments #3269

Riezebos opened this issue Feb 28, 2023 · 13 comments
Labels
needs-decision Awaiting a decision from a maintainer plugin Implementing a known but unsupported plugin

Comments

@Riezebos
Copy link

Calling a function with a lot of unnamed arguments is a lot less readable than using keyword arguments. These rules enforce keyword arguments if a function has a lot of parameters, which should lead to more readable code. I believe adding them to ruff would be a good idea!

https://github.com/isac322/flake8-force-keyword-arguments

If there is anything I can do for this as a Python dev with practically 0 Rust experience, I would be happy to help.

@charliermarsh
Copy link
Member

We haven't defined rigorous criteria for including rules in Ruff yet (#2257), but this rule does look a little niche just based on stars and usage, and I think it might be hard to enforce rigorously right now since we can't inspect function signatures across modules. So I'd vote to pass on this one for the moment just given the tradeoff in utility vs. complexity.

@charliermarsh charliermarsh added the plugin Implementing a known but unsupported plugin label Feb 28, 2023
@Riezebos
Copy link
Author

Riezebos commented Mar 1, 2023

Thanks for the response! I think it would be partially possible with only local information:

  • Rule 1: Flag any function definition that accepts >X positional arguments
  • Rule 2: Flag any function call that uses >X positional arguments

For the first rule I think only local information is needed? An autofix could potentially be provided that adds *, at the start of the parameter definition (and removes any other *, or /, from the definition).

With only local information rule 2 would falsely flag function calls of functions with >X positional-only parameters or with *args. I would personally be fine with manually adding noqa comments there but I can imagine that this disqualifies it.

Would rule 1 be an option?

@charliermarsh charliermarsh added the needs-decision Awaiting a decision from a maintainer label Jul 10, 2023
@flying-sheep
Copy link
Contributor

flying-sheep commented Oct 20, 2023

@charliermarsh I think you made a mistake when you filed pylint-dev/pylint#8667 and therefore PLR0913.

It’s much more useful in Python to restrict the number of positional arguments in function definitions.

In response to your change, a rule for only positional arguments has been requested for Pylint for that reason: pylint-dev/pylint#9099

@emilte
Copy link
Contributor

emilte commented Feb 1, 2024

I would love this.
I currently have a custom implementation of this in my project with pylint.
I'm considering to replace everything with ruff, but will surely miss this feature.
A star * at the beginning of functions would help immensely :)

@flying-sheep
Copy link
Contributor

R0917 aka “too-many-positional” is implemented in Ruff already.

@emilte
Copy link
Contributor

emilte commented Feb 2, 2024

Aha, that's great, thx!

I struggled a bit to get this working, is there a tiny bug in the documentation?
The example shows max-pos-args, but max-positional-args seems to be the correct key.
https://docs.astral.sh/ruff/settings/#pylint-max-positional-args

@dhruvmanila
Copy link
Member

The example shows max-pos-args, but max-positional-args seems to be the correct key.

Thanks! It must be a typo. Do you want to open a PR fixing the docs? The type is here:

#[option(default = r"3", value_type = "int", example = r"max-pos-args = 3")]

@emilte
Copy link
Contributor

emilte commented Feb 2, 2024

Thx for the opportunity.
#9797

@tmke8
Copy link
Contributor

tmke8 commented Feb 5, 2024

I think this issue can be closed then, since it's possible to achieve the desired behavior with PLR0917 and setting

[tool.ruff.lint.pylint]
max-positional-args = 1

@Riezebos Riezebos closed this as completed Feb 5, 2024
@jack-mcivor
Copy link
Contributor

@Riezebos are you sure this is closed?

Rule 1: Flag any function definition that accepts >X positional arguments
Rule 2: Flag any function call that uses >X positional arguments

AFAICT PLR0917 checks function definitions only (Rule 1). I don't think a check for function calls exists? (Rule 2)

@Riezebos
Copy link
Author

Riezebos commented Feb 6, 2024

@jack-mcivor you're right, I guess it is partially solved. I am not a contributor, and I am not the only one interested in these rules. So I don't think it is up to me anymore to decide whether this is open or closed.

  • Rule 1: Flag any function definition that accepts >X positional arguments (solved in PLR0917)
  • Rule 2: Flag any function call that uses >X positional arguments

Just to make the reference circle complete, this topic was also discusses in #8137

@uripeled2
Copy link

I would be very happy to see a rule that flags any function call that uses >X positional arguments

@Andrew-S-Rosen
Copy link

Andrew-S-Rosen commented Apr 15, 2024

Related, what would be super useful is some rule that would ensure that arguments are explicitly specified in keyword (rather than positional) format when calling a function that accepts keyword arguments. The purpose here is that if the order or number of arguments in the function signature is ever updated, the user's code would remain unaffected. This would, however, require inspection of each function signature, including those of dependencies.

This might not be viable in the short-term, but I wanted to throw out additional motivation beyond "it just looks nicer."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-decision Awaiting a decision from a maintainer plugin Implementing a known but unsupported plugin
Projects
None yet
Development

No branches or pull requests

9 participants