Customize arguments-differ error messages #4422

ksaketou · 2021-04-29T14:21:50Z

Steps

Add yourself to CONTRIBUTORS if you are a new contributor.
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 customizes the output messages of arguments-differ error message based on different error cases (type change, name change, number of parameters etc). It aims to act as a base for the creation of a new error message, arguments-renamed.

Type of Changes

	Type
✓	✨ New feature

Related Issue

Part one of #3536

This commit changes the output messages of arguments-differ based on different error cases. It is part one of the issue pylint-dev#3536

This commit modifies the tests of arguments-differ output messages based on different error messages. It is part one of the issue pylint-dev#3536

This commit modifies the tests for arguments-differ output messages based on different error cases. It is part one of the issue pylint-dev#3536

This file emited arguments-differ messages on three methods in the end of the file because some parameters have changed name compared to the original method.

coveralls · 2021-04-29T14:26:53Z

Coverage increased (+0.01%) to 91.696% when pulling 1ba9e1d on ksaketou:args-differ-msg into ce55bce on PyCQA:master.

Pierre-Sassoulas

Thank you, great work ! I suggested some small changes, the main thing being we should not change the existing tests, only their outputs. They're testing some use case that were constructed from dozen of issues and we need them to not regress. We can add other tests if required for typing check, having long functional test files with duplication is not a problem at all :)

Pierre-Sassoulas · 2021-04-29T15:54:57Z

tests/functional/a/arguments_differ.txt

+arguments-differ:41:4:ClassmethodChild.func:Number of parameters has changed in overridden 'ClassmethodChild.func' method
+arguments-differ:68:4:VarargsChild.has_kwargs:Parameter 'arg' was of type 'bool' and is now of type 'int' in overridden 'VarargsChild.has_kwargs' method
+arguments-differ:68:4:VarargsChild.has_kwargs:Variadics removed in overridden 'VarargsChild.has_kwargs' method
+arguments-differ:71:4:VarargsChild.no_kwargs:Parameter 'args' has been renamed in overridden 'VarargsChild.no_kwargs' method


Suggested change

arguments-differ:71:4:VarargsChild.no_kwargs:Parameter 'args' has been renamed in overridden 'VarargsChild.no_kwargs' method

arguments-differ:71:4:VarargsChild.no_kwargs:Parameter 'args' has been renamed to 'arg' in overridden 'VarargsChild.no_kwargs' method

Pierre-Sassoulas · 2021-04-29T15:56:43Z

tests/functional/a/arguments_differ.txt

+arguments-differ:68:4:VarargsChild.has_kwargs:Parameter 'arg' was of type 'bool' and is now of type 'int' in overridden 'VarargsChild.has_kwargs' method
+arguments-differ:68:4:VarargsChild.has_kwargs:Variadics removed in overridden 'VarargsChild.has_kwargs' method
+arguments-differ:71:4:VarargsChild.no_kwargs:Parameter 'args' has been renamed in overridden 'VarargsChild.no_kwargs' method
+arguments-differ:172:4:SecondChangesArgs.test:Number of parameters has changed in overridden 'SecondChangesArgs.test' method


Suggested change

arguments-differ:172:4:SecondChangesArgs.test:Number of parameters has changed in overridden 'SecondChangesArgs.test' method

arguments-differ:172:4:SecondChangesArgs.test:Number of parameters was 4 in 'Classname.base_method' and is now 5 in overridden 'SecondChangesArgs.test'

pylint/checkers/classes.py

Pierre-Sassoulas · 2021-04-29T16:01:31Z

pylint/checkers/classes.py

+    overridden: List[astroid.AssignName],
+    dummy_parameter_regex: Pattern,
+    counter: int,
+):


Here it was a boolean before it look like it's now a list of string.

Suggested change

):

) -> List[str]:

Pierre-Sassoulas · 2021-04-29T16:02:48Z

pylint/checkers/classes.py

-            return True
-    return False
+            result.append(
+                "Parameter '" + str(original_param.name) + "' has been renamed in"


Suggested change

"Parameter '" + str(original_param.name) + "' has been renamed in"

f"Parameter '{original_param.name}' has been renamed to '{overridden_param.name}' in"

Pierre-Sassoulas · 2021-04-29T16:05:35Z

pylint/checkers/classes.py

+    original: List[astroid.FunctionDef],
+    overridden: List[astroid.FunctionDef],
+    dummy_parameter_regex: Pattern,
+):


Suggested change

):

) -> List[str]:

Pierre-Sassoulas · 2021-04-29T16:05:57Z

pylint/checkers/classes.py

    )
+    if len(different_kwonly) > 0:


Suggested change

if len(different_kwonly) > 0:

if different_kwonly:

Pierre-Sassoulas · 2021-04-29T16:08:25Z

tests/functional/a/arguments_differ.py

@@ -3,13 +3,13 @@

 class Parent(object):

-    def test(self):


Could we refrain from changing the existing functional test in this MR, please ? I would like to see if the old use cases are all handled well. We can add more functional tests if required :)

ksaketou · 2021-04-30T07:56:01Z

Thank you! Okay I see, I'm working on these changes :)

This commit removes several changes that have been done on some method parameters, so that we keep the initial format of the tests.

This commit adds some changes on the output messages and also adds some more code for the compatibility with the initial tests.

for more information, see https://pre-commit.ci

Pierre-Sassoulas

Thank you , great work and very nice new message ! Could we also keep the typing change in functional tests for the next MR, please?

Pierre-Sassoulas · 2021-05-04T11:06:32Z

pylint/checkers/classes.py

+                        "Parameter '"
+                        + str(original_param.name)
+                        + "' was of type '"
+                        + original_type
+                        + "' and is now of type '"
+                        + overridden_type
+                        + "' in"


Suggested change

"Parameter '"

+ str(original_param.name)

+ "' was of type '"

+ original_type

+ "' and is now of type '"

+ overridden_type

+ "' in"

f "Parameter '{original_param.name}' was of type '{original_type}' and "

"is now of type '{overridden_type}' in"

…into args-differ-msg

for more information, see https://pre-commit.ci

ksaketou · 2021-05-04T20:09:49Z

Thank you , great work and very nice new message ! Could we also keep the typing change in functional tests for the next MR, please?

Thank you! What do you mean exactly? The parameter types on the tests?

Pierre-Sassoulas · 2021-05-04T19:52:22Z

ChangeLog

@@ -51,6 +51,8 @@ Release date: 2021-04-25

  Closes #4399

+* Customize output messages for ``arguments-differ`` error message based on the different error cases.


Suggested change

* Customize output messages for ``arguments-differ`` error message based on the different error cases.

* The warning for ``arguments-differ`` now signal explicitly the difference it detected by naming the argument or type that changed.

tests/functional/a/arguments_differ.py

for more information, see https://pre-commit.ci

Pierre-Sassoulas

It look like we're getting a problem in the pipeline because of #4439. I'll merge as soon as someone fix it and the test pass when we rebase. Thank you for all the work on this !

ksaketou · 2021-05-05T15:59:11Z

My pleasure! So I have to wait for this PR to be merged in order to start working on the second pull request?

Pierre-Sassoulas · 2021-05-05T16:46:08Z

Well, you can disregard the error and start the second one, we will rebase on master once the first one is merged. I also see that there is a problem with mypy that you could check before that: https://results.pre-commit.ci/run/github/47671127/1620160302.btadc94JQO-yT6SZWp2aLg

cdce8p · 2021-05-05T17:37:10Z

It look like we're getting a problem in the pipeline because of #4439. I'll merge as soon as someone fix it and the test pass when we rebase. Thank you for all the work on this !

The pylint problem here isn't related to the issue you linked. AFAIK it's because the annotation was assumed to have the name attribute. However that isn't always the case. It can also be of type astroid.Attribute which only has attrname.

https://github.com/PyCQA/pylint/blob/d05d2a18b57221c464c6d0799ba40bf66f306483/pylint/checkers/classes.py#L286-L290

ksaketou · 2021-05-05T18:03:47Z

I am working on the mypy errors, I think they might fix the problem. I had a small mistake on the parameter typing here.
https://github.com/PyCQA/pylint/blob/d05d2a18b57221c464c6d0799ba40bf66f306483/pylint/checkers/classes.py#L311-L312

This commit fixes the ignoring of special methods on function overriding and also removes incorrect use of the attribute name.

for more information, see https://pre-commit.ci

This commits creates the new error arguments-renamed based on the functionality added on pylint-dev#4422 and the changes of pylint-dev#4456.

* Create new error arguments-renamed This commits creates the new error arguments-renamed based on the functionality added on #4422 and the changes of #4456. * Merge test files for arguments-differ This commit merges the two kinds of test files that existed for the arguments-differ error, since now all tests run in Python3. * Add tests for arguments-renamed error * Add new error arguments-renamed Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>

ksaketou added 6 commits April 29, 2021 16:32

Customize output messages for arguments-differ message

d11a738

This commit changes the output messages of arguments-differ based on different error cases. It is part one of the issue pylint-dev#3536

Modify tests for arguments-differ

4faf101

This commit modifies the tests of arguments-differ output messages based on different error messages. It is part one of the issue pylint-dev#3536

Modify the arguments-differ tests for different output messages

9eb140b

This commit modifies the tests for arguments-differ output messages based on different error cases. It is part one of the issue pylint-dev#3536

Add customization of arguments-differ output messages

5a1c205

Resolve merge conflicts

d127ae7

Ignore arguments-differ messages

e92e510

This file emited arguments-differ messages on three methods in the end of the file because some parameters have changed name compared to the original method.

Improve code formatting

745d35f

Pierre-Sassoulas requested changes Apr 29, 2021

View reviewed changes

ksaketou and others added 5 commits April 30, 2021 16:43

Merge remote-tracking branch 'upstream/master' into args-differ-msg

b0ab6ec

Undo functions' parameters changes

b3c5f82

This commit removes several changes that have been done on some method parameters, so that we keep the initial format of the tests.

Add more details to error messages and adjust code to initial tests

33fca2e

This commit adds some changes on the output messages and also adds some more code for the compatibility with the initial tests.

Resolve merge conflict

4a5546d

[pre-commit.ci] auto fixes from pre-commit.com hooks

4599a4e

for more information, see https://pre-commit.ci

ksaketou requested a review from Pierre-Sassoulas May 4, 2021 10:56

Pierre-Sassoulas requested changes May 4, 2021

View reviewed changes

ksaketou and others added 3 commits May 4, 2021 23:01

Fix string format

836f341

Merge branch 'args-differ-msg' of https://github.com/ksaketou/pylint …

6c0dfa2

…into args-differ-msg

[pre-commit.ci] auto fixes from pre-commit.com hooks

0c0b9dd

for more information, see https://pre-commit.ci

Pierre-Sassoulas requested changes May 4, 2021

View reviewed changes

ksaketou and others added 2 commits May 4, 2021 23:30

Modify addition to ChangeLog

e04e2b2

[pre-commit.ci] auto fixes from pre-commit.com hooks

d05d2a1

for more information, see https://pre-commit.ci

Pierre-Sassoulas approved these changes May 5, 2021

View reviewed changes

Pierre-Sassoulas mentioned this pull request May 5, 2021

AttributeError: 'Subscript' object has no attribute 'name' #4439

Closed

Fix parameter typing on function declaration

60dbe32

ksaketou and others added 2 commits May 5, 2021 21:37

Fix mypy errors

f3b652d

This commit fixes the ignoring of special methods on function overriding and also removes incorrect use of the attribute name.

[pre-commit.ci] auto fixes from pre-commit.com hooks

1ba9e1d

for more information, see https://pre-commit.ci

Pierre-Sassoulas merged commit 7167442 into pylint-dev:master May 5, 2021

This was referenced May 6, 2021

Fix changelog #4442

Merged

Crash when checking function overwrites #4443

Closed

ksaketou deleted the args-differ-msg branch May 7, 2021 11:08

cdce8p mentioned this pull request May 10, 2021

Remove type check from arguments-differ #4456

Merged

ksaketou added a commit to ksaketou/pylint that referenced this pull request May 11, 2021

Create new error arguments-renamed

c5c5324

This commits creates the new error arguments-renamed based on the functionality added on pylint-dev#4422 and the changes of pylint-dev#4456.

ksaketou mentioned this pull request May 11, 2021

Create new error arguments-renamed #4467

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Customize arguments-differ error messages #4422

Customize arguments-differ error messages #4422

ksaketou commented Apr 29, 2021

coveralls commented Apr 29, 2021 •

edited

Pierre-Sassoulas left a comment

Pierre-Sassoulas Apr 29, 2021

Pierre-Sassoulas Apr 29, 2021

Pierre-Sassoulas Apr 29, 2021

Pierre-Sassoulas Apr 29, 2021

Pierre-Sassoulas Apr 29, 2021

Pierre-Sassoulas Apr 29, 2021

Pierre-Sassoulas Apr 29, 2021

ksaketou commented Apr 30, 2021

Pierre-Sassoulas left a comment

Pierre-Sassoulas May 4, 2021

ksaketou commented May 4, 2021

Pierre-Sassoulas May 4, 2021

Pierre-Sassoulas left a comment

ksaketou commented May 5, 2021

Pierre-Sassoulas commented May 5, 2021

cdce8p commented May 5, 2021

ksaketou commented May 5, 2021

	arguments-differ:71:4:VarargsChild.no_kwargs:Parameter 'args' has been renamed in overridden 'VarargsChild.no_kwargs' method
	arguments-differ:71:4:VarargsChild.no_kwargs:Parameter 'args' has been renamed to 'arg' in overridden 'VarargsChild.no_kwargs' method

	arguments-differ:172:4:SecondChangesArgs.test:Number of parameters has changed in overridden 'SecondChangesArgs.test' method
	arguments-differ:172:4:SecondChangesArgs.test:Number of parameters was 4 in 'Classname.base_method' and is now 5 in overridden 'SecondChangesArgs.test'

	"Parameter '" + str(original_param.name) + "' has been renamed in"
	f"Parameter '{original_param.name}' has been renamed to '{overridden_param.name}' in"

		@@ -51,6 +51,8 @@ Release date: 2021-04-25

		Closes #4399

		* Customize output messages for ``arguments-differ`` error message based on the different error cases.

	* Customize output messages for ``arguments-differ`` error message based on the different error cases.
	* The warning for ``arguments-differ`` now signal explicitly the difference it detected by naming the argument or type that changed.

Customize arguments-differ error messages #4422

Customize arguments-differ error messages #4422

Conversation

ksaketou commented Apr 29, 2021

Steps

Description

Type of Changes

Related Issue

coveralls commented Apr 29, 2021 • edited

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

ksaketou commented Apr 30, 2021

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksaketou commented May 4, 2021

Choose a reason for hiding this comment

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

ksaketou commented May 5, 2021

Pierre-Sassoulas commented May 5, 2021

cdce8p commented May 5, 2021

ksaketou commented May 5, 2021

coveralls commented Apr 29, 2021 •

edited