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

[feature request] New warning: too many positional arguments #9099

Closed
sabik opened this issue Oct 3, 2023 · 10 comments · Fixed by #9842 or #9940
Closed

[feature request] New warning: too many positional arguments #9099

sabik opened this issue Oct 3, 2023 · 10 comments · Fixed by #9842 or #9940
Labels
Enhancement ✨ Improvement to a component Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Milestone

Comments

@sabik
Copy link

sabik commented Oct 3, 2023

Current problem

With R0913 too-many-arguments now including keyword arguments (#8667), the reasonable setting for max-args is now larger, since it's not really a problem to have a dozen keyword-only arguments.

However, having a dozen positional arguments remains a poor design.

Desired solution

A new warning for too many positional arguments, with a separately configurable maximum. This would warn when there are more than a very small number of positional arguments (including positional-only arguments).

This would be in addition to the existing too-many-arguments warning.

Additional context

This has been previously suggested in comments on #8667

@sabik sabik added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Oct 3, 2023
@sabik sabik changed the title New warning: too many positional arguments [feature request] New warning: too many positional arguments Oct 3, 2023
@Pierre-Sassoulas Pierre-Sassoulas added Enhancement ✨ Improvement to a component Needs decision 🔒 Needs a decision before implemention or rejection and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Oct 3, 2023
@Pierre-Sassoulas Pierre-Sassoulas added this to the 3.1.0 milestone Oct 3, 2023
@jacobtylerwalls
Copy link
Member

+1

@Pierre-Sassoulas Pierre-Sassoulas added Needs PR This issue is accepted, sufficiently specified and now needs an implementation and removed Needs decision 🔒 Needs a decision before implemention or rejection labels Oct 3, 2023
@mbyrnepr2
Copy link
Member

As for specification would this proposed checker be for emitting a message where a function or class is called? (as opposed to too-many-arguments which is emitted for the function or class definition. It sounds similar to the existing #385.

@jacobtylerwalls
Copy link
Member

As I'm hearing the request, it's just on the definition (and that's what I would suggest, too).

@sabik
Copy link
Author

sabik commented Oct 3, 2023

Yeah, I intended this to be on the definition, just like the too-many-arguments warning (but excluding keyword-only arguments from the count and with a separate maximum)

@mbyrnepr2
Copy link
Member

I think the idea behind too-many-arguments is that, regardless of the argument being positional or keyword or positional-only (etc.), the function is complex by having too many of them (subjective I know :D) and doing too many things. At definition time this is possible because we have all the information we need (the number of params).

Would the new checker raise for this situation (where the function call has explicitly passed the names of the parameters)?

def tomato(one, two, three, four, five, six):
    ...

tomato(one=1, two=2, three=3, four=4, five=5, six=6)

Perhaps if it were to raise for positional-only, then raising at definition time is ok:

def tomato(one, two, three, four, five, six, /):
    ...

tomato(1, 2, 3, 4, 5, 6)

@sabik
Copy link
Author

sabik commented Oct 4, 2023

Yeah; the idea was that the new checker would raise for:

def tomato(one, two, three, four, five, six):
    ...

and also raise for:

def tomato(one, two, three, four, five, six, /):
    ...

It would be OK with

def tomato(one, two, *, three, four, five, six):
    ...

and OK with

def tomato(*, one, two, three, four, five, six):
    ...

In any case, I was thinking of definition time; the design of the function encourages calls with a confusing undifferentiated argument list, regardless of whether or not it's called that way (or called at all).

Call time would be more of a variant of #385, function actually being called with a confusing undifferentiated argument list, which is not what I was thinking of here...

@flying-sheep
Copy link
Contributor

I think #8667 was a mistake. “too many total arguments” could be a separate rule, but it’s much less useful than restricting the number of positional arguments.

@mbyrnepr2
Copy link
Member

I'm happy that 8667 sparked the conversation and that ambiguity will be removed with the introduction of the new checker.
I still feel that the spirit of too-many-arguments was to draw attention to a function that accepts a large number of arguments (irrespective of type) and to consider a re-design/refactor.

@tometzky
Copy link

Unfortunately, IMHO, significantly limiting number of keyword-only arguments only encourages bad practices, like:

  • using **kwargs or a dict argument and losing useful checks for typos in args, required args, arg type checking with mypy etc;
  • replacing arguments with a dataclasses.dataclass or typing.NamedTuple, but then soon be forced to disable too-many-instance-attributes;
  • over-engineering with some special classes like SomeFunctionArgumentGenerator - no way it is simpler and less error prone.

There are other checks in pylint that discourage too complicated code, like too-many-branches and too-many-locals etc. which should be enough.

With no obvious refactoring path for reasonable number of positional-only arguments, the change in #8667 made a useful check into a forced bunch of pylint: disable comments. Please revert #8667 until too-many-positional with a reasonable default is added (maybe 15, like too-many-locals).

@flying-sheep
Copy link
Contributor

far too late to go with your suggestion of reverting this, but a new code for it has been reserved: https://pylint.pycqa.org/en/latest/user_guide/messages/refactor/too-many-positional.html

you could either implement it here or switch to / add Ruff to your project

@jacobtylerwalls jacobtylerwalls added this to the 3.3.0 milestone Jul 29, 2024
DifferentialOrange added a commit to tarantool/tarantool-python that referenced this issue Sep 22, 2024
Pylint 3.3.0 includes a new warning: too-many-positional-arguments [1].
We already ignore a similar too-many-arguments warning in several places
since "fixing" it is backward-incompatible.

1. pylint-dev/pylint#9099
DifferentialOrange added a commit to tarantool/tarantool-python that referenced this issue Sep 22, 2024
Pylint 3.3.0 includes a new warning: too-many-positional-arguments [1].
We already ignore a similar too-many-arguments warning in several places
since "fixing" it is backward-incompatible.

1. pylint-dev/pylint#9099
DifferentialOrange added a commit to tarantool/tarantool-python that referenced this issue Sep 23, 2024
Pylint 3.3.0 includes a new warning: too-many-positional-arguments [1].
We already ignore a similar too-many-arguments warning in several places
since "fixing" it is backward-incompatible.

1. pylint-dev/pylint#9099
nmlorg added a commit to nmlorg/metabot that referenced this issue Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
None yet
6 participants