Fix a false negative for too-many-arguments and positional-only and keyword-only arguments
#8674
Conversation
mbyrnepr2
added
the
False Negative π¦
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #8674 /- ##
=======================================
Coverage 95.80% 95.80%
=======================================
Files 172 172
Lines 18301 18301
=======================================
Hits 17534 17534
Misses 767 767
|
|
The CI shows that the Pylint codebase has had this false negative hidden from sight in the ************* Module pylint.config.argument
[43](https://github.com/pylint-dev/pylint/actions/runs/4940248807/jobs/8831716489?pr=8674#step:7:44)
pylint/config/argument.py:232:4: R0913: Too many arguments (10/9) (too-many-arguments)
[44](https://github.com/pylint-dev/pylint/actions/runs/4940248807/jobs/8831716489?pr=8674#step:7:45)
pylint/config/argument.py:309:4: R0913: Too many arguments (10/9) (too-many-arguments)
[45](https://github.com/pylint-dev/pylint/actions/runs/4940248807/jobs/8831716489?pr=8674#step:7:46)
pylint/config/argument.py:357:4: R0913: Too many arguments (11/9) (too-many-arguments)
[46](https://github.com/pylint-dev/pylint/actions/runs/4940248807/jobs/8831716489?pr=8674#step:7:47)
pylint/config/argument.py:401:4: R0913: Too many arguments (10/9) (too-many-arguments)
[47](https://github.com/pylint-dev/pylint/actions/runs/4940248807/jobs/8831716489?pr=8674#step:7:48)
pylint/config/argument.py:438:4: R0913: Too many arguments (10/9) (too-many-arguments)This code would need a refactor in order to proceed with this merge-request. Looking at the above I notice an unrelated issue to the change in the current merge-request where too-many-arguments not emitted: def name1(param1, param2, param3, param4, param5):
...too-many-arguments is emitted: class A:
def name2(self, param1, param2, param3, param4, param5): # R0913: Too many arguments (6/5) (too-many-arguments)
... |
|
Let's fix the inconsistencies, but we can disable the existing violations in pylint. |
Ok @Pierre-Sassoulas! The refactoring may take some time to get right so I think that's a pragmatic approach. |
It also crossed my mind to do this but thought against it as we probably want our functions to trend towards the magic number (5 - the current default). Or do you think this default value should now also increase for all users? |
|
Created this merge-request since the primer comment is not showing up here. |
|
Thank you for fixing this, I was wondering why the primer was'nt working in some other MR ! |
|
Regarding the default we could use 7 to reduce disturbance following the false negative fix (we had to add a lot of disable ourselves, the primer could also show what to expect.). Also because 7 is the number of concept a human can keep in mind at the same time so it makes sense. |
β¦onal-only and keyword-only parameters. Closes pylint-dev#8667
mbyrnepr2
force-pushed
the
too_many_arguments_false_negative
branch
from
46ee4bf
to
5a6e5bb
Compare
This comment has been minimized.
This comment has been minimized.
|
We are missing 8 packages or so from the primer output because the comment size has reached its limit. |
|
Maybe we can revert 5a6e5bb in order to be able to see the newly raised messages in the primer ? |
|
It seems we're fixing much more than what was signaled in the original issue. In Black this was not detected before: https://github.com/psf/black/blob/64887aab032c0fd64f9238cdab6684f2fc0c7f33/src/black/__init__.py#L620 but there was more than 5 values already. We might want to explicitly add the test case so we do not regress later and upgrade the changelog, what do you think ? |
|
I'm wondering if we should get more input on the original request. It's not a violation of LSP to accept additional keyword-only arguments in subclasses, right? We already have |
jacobtylerwalls
added
the
Needs decision π
|
@Pierre-Sassoulas what the Here is the relevant debugging on the ************* Module black
> /Users/markbyrne/programming/pylint/pylint/checkers/design_analysis.py(511)visit_functiondef()
-> ignored_argument_names = self.linter.config.ignored_argument_names
(Pdb) node.args.args
[]
(Pdb) node.args
<Arguments l.622 at 0x1121b0160>
(Pdb) node.args.args
[]
(Pdb) node.args.kwonlyargs
[<AssignName.ctx l.622 at 0x1121b01f0>, <AssignName.src l.623 at 0x1121b0220>, <AssignName.quiet l.624 at 0x1121b0250>, <AssignName.verbose l.625 at 0x1121b0280>, <AssignName.include l.626 at 0x1121b02b0>, <AssignName.exclude l.627 at 0x1121b02e0>, <AssignName.extend_exclude l.628 at 0x1121b0310>, <AssignName.force_exclude l.629 at 0x1121b0340>, <AssignName.report l.630 at 0x1121b0370>, <AssignName.stdin_filename l.631 at 0x1121b03a0>]
(Pdb) node.args.posonlyargs
[]Here is the function for reference: def get_sources(
*,
ctx: click.Context,
src: Tuple[str, ...],
quiet: bool,
verbose: bool,
include: Pattern[str],
exclude: Optional[Pattern[str]],
extend_exclude: Optional[Pattern[str]],
force_exclude: Optional[Pattern[str]],
report: "Report",
stdin_filename: Optional[str],
)So, Pylint sees nothing before the |
|
@jacobtylerwalls I don't think we are on the same page about the rationale for I think we can always rely on teams configuring the value for Some notes on too-many-arguments configuration of our current primer packages which do use Pylint: |
jacobtylerwalls
removed
the
Needs decision π
|
@mbyrnepr2 got it now, thank you! I had forgotten that we don't use the primer project's own pylintrc with respect to enables/disables. |
Type of Changes
Description
Currently the logic behind the
too-many-argumentschecker does not consider positional-only and keyword-only arguments. This fix factors those arguments into the equation.Closes #8667