Separate functionality of arguments-differ with new error message called arguments-renamed #4347

ksaketou · 2021-04-13T08:34:06Z

Steps

Add a ChangeLog entry describing what your PR does.
If it's a new feature or an important bug fix, add a What's New entry in
doc/whatsnew/<current release.rst>.
Write a good description on what the PR does.

Description

This pull request separates the functionality of error message arguments-differ in two errors.
It creates a new error message called arguments-renamed which checks if the argument name of an overridden function has changed compared to the original, without having changed type. arguments-differ checks the type of args, **kwargs, number of args etc without including the argument names.

Type of Changes

	Type
✓	✨ New feature
✓	📜 Docs

Related Issue

Closes #3536

This commit adds a new error message called arguments-renamed. It is raised when an overridden function's argument has been renamed compared to the original without having changed type. The commit also removes the functionality from arguments-differ of checking if the argument names have changed.

This commit updates the arguments-differ test by removing expected errors where only the name has changed as this will be tracked from arguments-renamed. It also adds a few more test cases.

This commit adds the test for the arguments-renamed error message. The test checks in different cases if the argument names of overridden functions have changed without changing type compared to the original function.

This commit adds the new error message arguments-renamed on ChangeLog. This error was created to separate functionality of arguments-differ error message.

Pierre-Sassoulas

Thank you for working on this ! Would it be possible to do a refactoring MR first, for the removal of dummy_parameter_regex, please ? It would make the change more reviewable :)

pylint/checkers/classes.py

Pierre-Sassoulas · 2021-04-13T12:06:32Z

tests/functional/a/arguments_differ.py

@@ -1,5 +1,5 @@
 """Test that we are emitting arguments-differ when the arguments are different."""
-# pylint: disable=missing-docstring, too-few-public-methods, unused-argument,useless-super-delegation, useless-object-inheritance
+# pylint: disable= arguments-renamed, missing-docstring, too-few-public-methods, unused-argument,useless-super-delegation, useless-object-inheritance


What is the new result of this file if we keep it the same than before? It's just to picture easily what changed in this MR :) (thinking is hard)

I just added that so that the test emit only the argument-differ errors. I think it is less complicated and easier to understand the test output for arguments-differ.

Pierre-Sassoulas · 2021-04-13T12:09:28Z

tests/functional/a/arguments_renamed.py

+
+class ChildDefaults(ParentDefaults):
+
+    def test1(self, param1: str, param2: str): # [arguments-renamed]


Suggested change

def test1(self, param1: str, param2: str): # [arguments-renamed]

def test1(self, param1: str, param2: str): # [arguments-renamed, argument-renamed]

I think the expected result is two messages. One with arg renamed to param1 and one with barg renamed to param2 in the message. What do you think ?

Since the output message is "Parameters have been renamed" and not "Parameter" and also it doesn't specify the arguments, so I believe one message is enough.

Pierre-Sassoulas · 2021-04-13T12:11:15Z

tests/functional/a/arguments_renamed.py

+    def test2(self, param1: int, param2: bool, param3: int): # no arguments-renamed here
+        print(f"Argument values are {param1}, {param2} and {param3}!")
+
+    def test3(self, param1: bool, arg2: bool): # no arguments-renamed here


arg1 was renamed to param1 though ?

yes but the type has changed, so I think it is considered arguments-differ. It would be arguments-renamed only if the name had changed and not the type

Pierre-Sassoulas · 2021-04-13T12:12:29Z

tests/functional/a/arguments_renamed.py

+    def test1(self, param1: str, param2: str): # [arguments-renamed]
+        print(f"Argument values are {param1} and {param2}")
+
+    def test2(self, param1: int, param2: bool, param3: int): # no arguments-renamed here


Haven't the two first arguments been renamed ? Even the type changed.

Same here, the type has changed too so I think it is considered an arguments-differ, unless I understood something wrong

Pierre-Sassoulas · 2021-04-13T12:13:15Z

tests/functional/a/arguments_renamed.py

+# pylint: disable=arguments-differ, missing-docstring, no-self-use, too-few-public-methods, useless-object-inheritance, line-too-long
+"""Test where we are emitting arguments-renamed when the
+    arguments have been renamed without changing type.
+"""


Nice functional tests ! (although I kinda miss the fruity example 😄 )

Pierre-Sassoulas · 2021-04-13T12:15:32Z

tests/functional/a/arguments_differ.txt

@@ -4,3 +4,4 @@ arguments-differ:41:4:ClassmethodChild.func:Parameters differ from overridden 'f
 arguments-differ:68:4:VarargsChild.has_kwargs:Parameters differ from overridden 'has_kwargs' method
 arguments-differ:71:4:VarargsChild.no_kwargs:Parameters differ from overridden 'no_kwargs' method
 arguments-differ:172:4:SecondChangesArgs.test:Parameters differ from overridden 'test' method
+arguments-differ:239:4:ChildClass2.meth:Parameters differ from overridden 'meth' method


Probably not related to the current MR but that would be nice to have:

Suggested change

arguments-differ:239:4:ChildClass2.meth:Parameters differ from overridden 'meth' method

arguments-differ:239:4:ChildClass2.meth:Parameter 'arg' was of type 'int' in 'ParentClass' and is now a 'bool' in overridden 'ChildClass2.meth' method

Could we do something similar in argument-renamed ?

Nice idea @Pierre-Sassoulas , but at that case I think we would have multiple cases for *args, **kwargs, argument removals, additions etc. Maybe a bit complicated? For the arguments-renamed it's more simple, maybe we could say for example :
arguments-renamed:200:4:ChildClass2.meth:Parameter 'arg' in 'ParentClass' has been renamed to 'arg1' in overridden 'ChildClass2.meth' method

ksaketou · 2021-04-14T08:31:38Z

Thank you for working on this ! Would it be possible to do a refactoring MR first, for the removal of dummy_parameter_regex, please ? It would make the change more reviewable :)

From what I understood, the dummy_parameter_regex is necessary for the current functionality of arguments-differ (before the creation of arguments-renamed), because I tried to remove it but the tests fail. On arguments-differ we dont check the type of the arguments at all and we only check the names, so I think the regex serves some kind of purpose (which I haven't fully understood).

ksaketou · 2021-04-24T14:49:41Z

Hello @Pierre-Sassoulas . I am wondering how do you want me to keep working on that PR? As I mentioned, I cannot remove the dummy_parameter_regex from the current version of the code (without the arguments-renamed error), so I cannot create a separate pull request for that. 😢 I added the argument types on the functions as you suggested. Do you want me to create some tests for Python3 so that more tests pass?

Pierre-Sassoulas · 2021-04-24T16:25:13Z

I think this become too huge of a change and that we'll have to separate the MR in two. The first step could be to make a better message for arguments-differ (something like : #4347 (comment)) on the numerous use cases we created here. Then the seconde step could be rebasing on it and we'd be able to check that arguments-differ stays coherent and that the new message arguments-renamed have a similar detailed message in the proper cases. What do you think ?

ksaketou · 2021-04-25T16:00:03Z

Great, so the first pull request will change the output message for arguments-differ as you suggested, without changing its functionality, and the second pull request will be the creation of arguments-renamed , right?
Also, how will we manage the messages for arguments-differ? They cannot have only the format you suggested since this error covers many cases (change of type, less/more parameters than the original method, kwargs etc)

Pierre-Sassoulas · 2021-04-25T19:12:21Z

Great, so the first pull request will change the output message for arguments-differ as you suggested, without changing its functionality, and the second pull request will be the creation of arguments-renamed , right?

👍

Regarding the message, I trust you to create a descriptive message for each case 😄 I think having the name of arguments that differ and what differ if it's the type is really helpful, but the exact wording is not that important.

ksaketou · 2021-04-26T06:23:51Z

Okay nice, I will do my best then 😀

ksaketou · 2021-04-29T08:36:51Z

Hello again, I have another question. I have written some code and now I am running all the tests with python -m pytest but the benchmark tests are failing. Does that have to do with the issue I am working on?

Pierre-Sassoulas · 2021-04-29T09:35:23Z

Did you install pytest-benchmark ? :)

ksaketou · 2021-04-29T13:29:12Z

Okay it runs now, thanks 😅

ksaketou added 6 commits April 13, 2021 10:56

Update test without argument name check

b32d445

This commit updates the arguments-differ test by removing expected errors where only the name has changed as this will be tracked from arguments-renamed. It also adds a few more test cases.

Add new test for arguments-renamed

8b2501f

This commit adds the test for the arguments-renamed error message. The test checks in different cases if the argument names of overridden functions have changed without changing type compared to the original function.

fix merge conflicts

7fdcf3d

add new error message arguments-renamed

e3324b1

This commit adds the new error message arguments-renamed on ChangeLog. This error was created to separate functionality of arguments-differ error message.

Add arguments-renamed new error message

8fe08b3

Pierre-Sassoulas requested changes Apr 13, 2021

View reviewed changes

Pierre-Sassoulas added Checkers Related to a checker Enhancement ✨ Improvement to a component labels Apr 13, 2021

add typing on function signature

87fe230

ksaketou requested a review from Pierre-Sassoulas April 24, 2021 14:49

ksaketou closed this May 17, 2021

ksaketou deleted the issue-3536-arg-differ branch May 17, 2021 13:29

Pierre-Sassoulas mentioned this pull request Aug 28, 2021

Settings to tolerate optional arguments #4903

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate functionality of arguments-differ with new error message called arguments-renamed #4347

Separate functionality of arguments-differ with new error message called arguments-renamed #4347

ksaketou commented Apr 13, 2021

Pierre-Sassoulas left a comment

Pierre-Sassoulas Apr 13, 2021

ksaketou Apr 14, 2021

Pierre-Sassoulas Apr 13, 2021

ksaketou Apr 14, 2021

Pierre-Sassoulas Apr 13, 2021

ksaketou Apr 14, 2021

Pierre-Sassoulas Apr 13, 2021

ksaketou Apr 14, 2021

Pierre-Sassoulas Apr 13, 2021

Pierre-Sassoulas Apr 13, 2021

ksaketou Apr 14, 2021 •

edited

ksaketou commented Apr 14, 2021 •

edited

ksaketou commented Apr 24, 2021

Pierre-Sassoulas commented Apr 24, 2021

ksaketou commented Apr 25, 2021

Pierre-Sassoulas commented Apr 25, 2021

ksaketou commented Apr 26, 2021

ksaketou commented Apr 29, 2021

Pierre-Sassoulas commented Apr 29, 2021

ksaketou commented Apr 29, 2021


		class ChildDefaults(ParentDefaults):

		def test1(self, param1: str, param2: str): # [arguments-renamed]

	arguments-differ:239:4:ChildClass2.meth:Parameters differ from overridden 'meth' method
	arguments-differ:239:4:ChildClass2.meth:Parameter 'arg' was of type 'int' in 'ParentClass' and is now a 'bool' in overridden 'ChildClass2.meth' method

Separate functionality of arguments-differ with new error message called arguments-renamed #4347

Separate functionality of arguments-differ with new error message called arguments-renamed #4347

Conversation

ksaketou commented Apr 13, 2021

Steps

Description

Type of Changes

Related Issue

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksaketou Apr 14, 2021 • edited

Choose a reason for hiding this comment

ksaketou commented Apr 14, 2021 • edited

ksaketou commented Apr 24, 2021

Pierre-Sassoulas commented Apr 24, 2021

ksaketou commented Apr 25, 2021

Pierre-Sassoulas commented Apr 25, 2021

ksaketou commented Apr 26, 2021

ksaketou commented Apr 29, 2021

Pierre-Sassoulas commented Apr 29, 2021

ksaketou commented Apr 29, 2021

ksaketou Apr 14, 2021 •

edited

ksaketou commented Apr 14, 2021 •

edited