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

Customize arguments-differ error messages #4422

Merged
merged 20 commits into from
May 5, 2021
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
d11a738
Customize output messages for arguments-differ message
ksaketou Apr 29, 2021
4faf101
Modify tests for arguments-differ
ksaketou Apr 29, 2021
9eb140b
Modify the arguments-differ tests for different output messages
ksaketou Apr 29, 2021
5a1c205
Add customization of arguments-differ output messages
ksaketou Apr 29, 2021
d127ae7
Resolve merge conflicts
ksaketou Apr 29, 2021
e92e510
Ignore arguments-differ messages
ksaketou Apr 29, 2021
745d35f
Improve code formatting
ksaketou Apr 29, 2021
b0ab6ec
Merge remote-tracking branch 'upstream/master' into args-differ-msg
ksaketou Apr 30, 2021
b3c5f82
Undo functions' parameters changes
ksaketou May 4, 2021
33fca2e
Add more details to error messages and adjust code to initial tests
ksaketou May 4, 2021
4a5546d
Resolve merge conflict
ksaketou May 4, 2021
4599a4e
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] May 4, 2021
836f341
Fix string format
ksaketou May 4, 2021
6c0dfa2
Merge branch 'args-differ-msg' of https://github.com/ksaketou/pylint …
ksaketou May 4, 2021
0c0b9dd
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] May 4, 2021
e04e2b2
Modify addition to ChangeLog
ksaketou May 4, 2021
d05d2a1
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] May 4, 2021
60dbe32
Fix parameter typing on function declaration
ksaketou May 5, 2021
f3b652d
Fix mypy errors
ksaketou May 5, 2021
1ba9e1d
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] May 5, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ Release date: 2021-04-25

Closes #4399

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


What's New in Pylint 2.8.0?
===========================
Expand Down
2 changes: 2 additions & 0 deletions doc/whatsnew/2.9.rst
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,5 @@ Other Changes

* Fix false-positive ``too-many-ancestors`` when inheriting from builtin classes,
especially from the ``collections.abc`` module

* The output messages for ``arguments-differ`` error message have been customized based on the different error cases.
72 changes: 54 additions & 18 deletions pylint/checkers/classes.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
"""
import collections
from itertools import chain, zip_longest
from typing import List, Pattern

import astroid

Expand Down Expand Up @@ -266,22 +267,40 @@ def _has_different_parameters_default_value(original, overridden):
return False


def _has_different_parameters(original, overridden, dummy_parameter_regex):
def _has_different_parameters(original: List[astroid.AssignName], overridden: List[astroid.AssignName],
dummy_parameter_regex: Pattern, counter: int):
result = []
zipped = zip_longest(original, overridden)
for original_param, overridden_param in zipped:
params = (original_param, overridden_param)
for original_param, overridden_param in zipped:
params = (original_param, overridden_param)
if not all(params):
return True

return ["Number of parameters has changed in"]

# check for the arguments' type
original_type = original_param.parent.annotations[counter]
if original_type is not None:
original_type = str(original_param.parent.annotations[counter].name)

overridden_type = overridden_param.parent.annotations[counter]
if overridden_type is not None:
overridden_type = str(overridden_param.parent.annotations[counter].name)
if original_type != overridden_type:
result.append("Parameter '"+ str(original_param.name) + "' was of type '"+ original_type +
"' and is now of type '"+ overridden_type + "' in")
counter += 1

#check for the arguments' name
names = [param.name for param in params]
if any(dummy_parameter_regex.match(name) for name in names):
continue
if original_param.name != overridden_param.name:
return True
return False
result.append("Parameter '"+ str(original_param.name) +"' has been renamed in")

return result

def _different_parameters(original, overridden, dummy_parameter_regex):

def _different_parameters(original: List[astroid.FunctionDef], overridden:
List[astroid.FunctionDef], dummy_parameter_regex: Pattern):
"""Determine if the two methods have different parameters

They are considered to have different parameters if:
Expand All @@ -293,6 +312,7 @@ def _different_parameters(original, overridden, dummy_parameter_regex):
* they have different keyword only parameters.

"""
output_messages = []
original_parameters = _positional_parameters(original)
overridden_parameters = _positional_parameters(overridden)

Expand All @@ -315,12 +335,25 @@ def _different_parameters(original, overridden, dummy_parameter_regex):
v for v in original.args.kwonlyargs if v.name in overidden_names
]

arguments = list(original.args.args)
# variable 'count' helps to check if the type of an argument has changed
# at the _has_different_parameters method
if any(arg.name == "self" for arg in arguments) and len(arguments) > 1:
count = 1
else:
count = 0

different_positional = _has_different_parameters(
original_parameters, overridden_parameters, dummy_parameter_regex
original_parameters, overridden_parameters, dummy_parameter_regex, count
)
different_kwonly = _has_different_parameters(
original_kwonlyargs, overridden.args.kwonlyargs, dummy_parameter_regex
original_kwonlyargs, overridden.args.kwonlyargs, dummy_parameter_regex, count
)
if len(different_kwonly) > 0:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if len(different_kwonly) > 0:
if different_kwonly:

output_messages += different_kwonly
if len(different_positional) > 0:
output_messages += different_positional

if original.name in PYMETHODS:
# Ignore the difference for special methods. If the parameter
# numbers are different, then that is going to be caught by
Expand All @@ -334,7 +367,10 @@ def _different_parameters(original, overridden, dummy_parameter_regex):
kwarg_lost = original.args.kwarg and not overridden.args.kwarg
vararg_lost = original.args.vararg and not overridden.args.vararg

return any((different_positional, kwarg_lost, vararg_lost, different_kwonly))
if kwarg_lost or vararg_lost:
output_messages += ["Variadics removed in"]

return output_messages


def _is_invalid_base_class(cls):
Expand Down Expand Up @@ -549,7 +585,7 @@ def _has_same_layout_slots(slots, assigned_value):
"be written as a function.",
),
"W0221": (
"Parameters differ from %s %r method",
"%s %s %r method",
"arguments-differ",
"Used when a method has a different number of arguments than in "
"the implemented interface or in an overridden method.",
Expand Down Expand Up @@ -1760,12 +1796,12 @@ def _check_signature(self, method1, refmethod, class_type, cls):
if is_property_setter(method1):
return

if _different_parameters(
refmethod, method1, dummy_parameter_regex=self._dummy_rgx
):
self.add_message(
"arguments-differ", args=(class_type, method1.name), node=method1
)
arg_differ_output = _different_parameters(refmethod, method1, dummy_parameter_regex=self._dummy_rgx)
if len(arg_differ_output) > 0:
for msg in arg_differ_output:
self.add_message(
"arguments-differ", args=(msg, class_type, str(method1.parent.name) + "." + str(method1.name)), node=method1
)
elif (
len(method1.args.defaults) < len(refmethod.args.defaults)
and not method1.args.vararg
Expand Down
34 changes: 18 additions & 16 deletions tests/functional/a/arguments_differ.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@

class Parent(object):

def test(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

def test(self, arg: bool):
pass


class Child(Parent):

def test(self, arg): # [arguments-differ]
def test(self, arg: int): #[arguments-differ]
pass


Expand All @@ -27,7 +27,7 @@ def test(self, arg=None): # [arguments-differ]
class Classmethod(object):

@classmethod
def func(cls, data):
def func(cls, data: str):
Pierre-Sassoulas marked this conversation as resolved.
Show resolved Hide resolved
return data

@classmethod
Expand Down Expand Up @@ -56,20 +56,20 @@ def fromkeys(cls, arg, arg1):

class Varargs(object):

def has_kwargs(self, arg, **kwargs):
def has_kwargs(self, arg: bool, **kwargs):
pass

def no_kwargs(self, args):
def no_kwargs(self, args: bool):
pass


class VarargsChild(Varargs):

def has_kwargs(self, arg): # [arguments-differ]
"Not okay to lose capabilities."
def has_kwargs(self, arg: int): #[arguments-differ, arguments-differ]
"Not okay to lose capabilities. Also, type has changed."

def no_kwargs(self, arg, **kwargs): # [arguments-differ]
"Not okay to add extra capabilities."
def no_kwargs(self, arg: bool, **kwargs): # [arguments-differ]
"Addition of kwargs does not violate LSP, but first argument's name has changed."


class Super(object):
Expand Down Expand Up @@ -111,14 +111,14 @@ def method(self, param='abc'):
class Staticmethod(object):

@staticmethod
def func(data):
def func(data: int):
return data


class StaticmethodChild(Staticmethod):

@classmethod
def func(cls, data):
def func(cls, data: str):
return data


Expand Down Expand Up @@ -169,7 +169,7 @@ def test(self, *args):

class SecondChangesArgs(FirstHasArgs):

def test(self, first, second, *args): # [arguments-differ]
def test(self, first: int, second: int, *args): # [arguments-differ]
pass


Expand Down Expand Up @@ -213,23 +213,25 @@ def mixed(self, first, *args, third, **kwargs):

class HasSpecialMethod(object):

def __getitem__(self, key):
def __getitem__(self, key: int):
return key


class OverridesSpecialMethod(HasSpecialMethod):

def __getitem__(self, cheie):
def __getitem__(self, cheie: int): #[arguments-differ]
return cheie + 1


class ParentClass(object):

def meth(self, arg, arg1):
def meth(self, arg: str, arg1: str):
raise NotImplementedError


class ChildClass(ParentClass):

def meth(self, _arg, dummy):
def meth(self, _arg: str, dummy: str):
# no error here, "dummy" and "_" are being ignored if
# spotted in a variable name (declared in dummy_parameter_regex)
pass
14 changes: 8 additions & 6 deletions tests/functional/a/arguments_differ.txt
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
arguments-differ:12:4:Child.test:Parameters differ from overridden 'test' method
arguments-differ:23:4:ChildDefaults.test:Parameters differ from overridden 'test' method
arguments-differ:41:4:ClassmethodChild.func:Parameters differ from overridden 'func' method
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:12:4:Child.test:Parameter 'arg' was of type 'bool' and is now of type 'int' in overridden 'Child.test' method
arguments-differ:23:4:ChildDefaults.test:Number of parameters has changed in overridden 'ChildDefaults.test' method
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

arguments-differ:222:4:OverridesSpecialMethod.__getitem__:Parameter 'key' has been renamed in overridden 'OverridesSpecialMethod.__getitem__' method
22 changes: 11 additions & 11 deletions tests/functional/a/arguments_differ_py3.py
Original file line number Diff line number Diff line change
@@ -1,40 +1,40 @@
# pylint: disable=missing-docstring,too-few-public-methods
class AbstractFoo:

def kwonly_1(self, first, *, second, third):
def kwonly_1(self, first: int, *, second: int, third: int):
"Normal positional with two positional only params."

def kwonly_2(self, *, first, second):
def kwonly_2(self, *, first: str, second: str):
"Two positional only parameter."

def kwonly_3(self, *, first, second):
def kwonly_3(self, *, first: str, second: str):
"Two positional only params."

def kwonly_4(self, *, first, second=None):
def kwonly_4(self, *, first: str, second=None):
"One positional only and another with a default."

def kwonly_5(self, *, first, **kwargs):
def kwonly_5(self, *, first: bool, **kwargs):
"Keyword only and keyword variadics."

def kwonly_6(self, first, second, *, third):
def kwonly_6(self, first: float, second: float, *, third: int):
"Two positional and one keyword"


class Foo(AbstractFoo):

def kwonly_1(self, first, *, second): # [arguments-differ]
def kwonly_1(self, first: int, *, second: int): # [arguments-differ]
"One positional and only one positional only param."

def kwonly_2(self, first): # [arguments-differ]
def kwonly_2(self, *, first: str): # [arguments-differ]
"Only one positional parameter instead of two positional only parameters."

def kwonly_3(self, first, second): # [arguments-differ]
def kwonly_3(self, **kwargs):
"Two positional params."

def kwonly_4(self, first, second): # [arguments-differ]
def kwonly_4(self, *args): # [arguments-differ]
"Two positional params."

def kwonly_5(self, *, first): # [arguments-differ]
def kwonly_5(self, *, first: bool): # [arguments-differ]
"Keyword only, but no variadics."

def kwonly_6(self, *args, **kwargs): # valid override
Expand Down
9 changes: 4 additions & 5 deletions tests/functional/a/arguments_differ_py3.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
arguments-differ:25:4:Foo.kwonly_1:Parameters differ from overridden 'kwonly_1' method
arguments-differ:28:4:Foo.kwonly_2:Parameters differ from overridden 'kwonly_2' method
arguments-differ:31:4:Foo.kwonly_3:Parameters differ from overridden 'kwonly_3' method
arguments-differ:34:4:Foo.kwonly_4:Parameters differ from overridden 'kwonly_4' method
arguments-differ:37:4:Foo.kwonly_5:Parameters differ from overridden 'kwonly_5' method
arguments-differ:25:4:Foo.kwonly_1:Number of parameters has changed in overridden 'Foo.kwonly_1' method
arguments-differ:28:4:Foo.kwonly_2:Number of parameters has changed in overridden 'Foo.kwonly_2' method
arguments-differ:34:4:Foo.kwonly_4:Number of parameters has changed in overridden 'Foo.kwonly_4' method
arguments-differ:37:4:Foo.kwonly_5:Variadics removed in overridden 'Foo.kwonly_5' method
2 changes: 1 addition & 1 deletion tests/functional/t/too/too_many_ancestors.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# pylint: disable=missing-docstring, too-few-public-methods, useless-object-inheritance
# pylint: disable=missing-docstring, too-few-public-methods, useless-object-inheritance, arguments-differ
from collections.abc import MutableSequence

class Aaaa(object):
Expand Down