Bug1085 #1618
Bug1085 #1618
Conversation
Conflicts: pylint/checkers/classes.py
rogalski
left a comment
rogalski
left a comment
There was a problem hiding this comment.
One minor warning from pylint - please fix that.
Overall looks good, I left some comments inlined.
pylint/checkers/classes.py
Outdated
| orig_default = None | ||
| try: | ||
| over_default = overridden.default_value(param_name).value | ||
| if orig_default != over_default: |
There was a problem hiding this comment.
What happens when default value is None?
Is overridden.default_value(param_name).value equal to None or equal to nodes.Const(None)? Maybe we can test for that? (default value equal to None is very common).
There was a problem hiding this comment.
Nice remark. My code fails if the default value is equal to None.
I'm adding a test and working on a fix.
| # we have found the method but it's not in the local | ||
| # dictionary. | ||
| # This may happen with astroid build from living objects | ||
| continue |
There was a problem hiding this comment.
this branch is not taken in tests - it would be cool to have this covered by some automated test, but I guess we do not have to be that restrictive.
There was a problem hiding this comment.
In fact, i just copied/pasted code of the visit_functiondef...
pylint/checkers/classes.py
Outdated
| # This may happen with astroid build from living objects | ||
| continue | ||
| if not isinstance(meth_node, astroid.FunctionDef): | ||
| continue |
Conflicts: ChangeLog
…nt_parameters_default_value method where instead of .value pty the method as_string is used. Adding test for default values that are int.
rogalski
left a comment
rogalski
left a comment
There was a problem hiding this comment.
IMHO it's really closed to being merged. See my comments and decide whether they seems reasonable to you.
pylint/checkers/classes.py
Outdated
| Return True if one of the overridden arguments has a default | ||
| value different from the default value of the original argument | ||
| """ | ||
| orig_name = [param.name for param in original.args] |
There was a problem hiding this comment.
.as_string() is not kind of API you want to use in this case.
Maybe consider using standard sentinel notion. Something around the lines of:
default_missing = object()
for param_name in orig_name:
try:
orig_default = original.default_value(param_name)
except astroid.exceptions.NoDefault:
orig_default = default_missing
try:
over_default = overridden.default_value(param_name)
except astroid.exceptions.NoDefault:
over_default = default_missing
if orig_default != over_default
return True
return False===========================
- the check of args.args is None is done inside ``_has_different_parameters_default_value``
- the call to ``_check_useless_super_delegation`` in ``visit_functiondef`` method has been reordered in order
to avoid the use of the ``_check_different_default_values_in_overridden_method`` method that had copy/pasted code
which was also not tested.
- deletion of ``_check_different_default_values_in_overridden_method`` which is now useless
- change in the comment of ``_check_useless_super_delegation`` method to indicate that this method
doesn't look into the value of default arguments to say delegation is useless
pylint/test/functional/useless_super_delegation.py
===================================================
- add of test with fake method which doesn't really override base method
…al names more comprehensives)
…e, then use as_string() method. Add of a functional test with default value that is not a literal but ca ClassDef (tuple)
|
I finally removed the method |
|
I don't understand the coverage failure. I just changed the name of a local variable by removing a letter. |
|
@PCManticore Please could we reauthorize coveralls with Github. I can't see why coveralls is failing at the moment. |
rogalski
added
Blocked 🚧
|
Marked as blocked by coverage failure.
|
|
Should i do something special in Github ? |
|
Unfortunately this can only be done by @PCManticore |
|
Ok thanks @AWhetter. |
|
This should work now, thanks for letting me know! |
PCManticore
left a comment
PCManticore
left a comment
There was a problem hiding this comment.
There are a couple of minor things left to do, but the major one out of it is the actual place of the new _has_different_default_param_values function call, which is currently not where it should be.
pylint/checkers/classes.py
Outdated
| continue | ||
| if not isinstance(meth_node, astroid.FunctionDef): | ||
| continue | ||
| # Don't care about functions with unknown argument (builtins). |
There was a problem hiding this comment.
I think the comment is stale
| c-extensions. | ||
| Close #1466 | ||
|
|
||
| * Fix a false positive ``useless-super-delegation`` message when |
There was a problem hiding this comment.
This will also need a What's new Entry, in the Other Changes section. Also please add yourself to Contributors file if you are not already there.
pylint/checkers/classes.py
Outdated
| # if value attribute doesn't exist, it may be due to the fact | ||
| # that the default value is a ClassDef object and then the default | ||
| # value is taken to be the string representation | ||
| original_default = original.default_value(param_name).as_string() |
There was a problem hiding this comment.
I would say we should refactor this a bit to avoid some potential expensive calls:
- just get the default value, without accessing
.valuefor both of the fields. thus when getting to line 165, we had noas_stringcall thus far - there, check for common node kinds, by verifying their type. E.g, are these two Consts? get their value and check against that. Are these classes, get their name and check against that. We might throw the class type in there as well, so we can differentiate between List and Tuples for example. Calling
as_stringmight be potentially slow which is why I prefer to avoid doing it unless necessary
pylint/checkers/classes.py
Outdated
| if not isinstance(meth_node, astroid.FunctionDef): | ||
| continue | ||
| # Don't care about functions with unknown argument (builtins). | ||
| if not _has_different_parameters_default_value(meth_node.args, node.args): |
There was a problem hiding this comment.
I'm not exactly sure this makes sense. Basically we can potentially call _check_useless_super_delegation for each overridden method, which can be many in certain rich hierarchy classes. This makes me think that the _has_different_parameters_default_value shouldn't actually be here, but rather in _check_useless_super_delegation itself
There was a problem hiding this comment.
In fact, i put it here to avoid code duplicate but you are definitely right it make more sense to put it inside _check_useless_super_delegation itself.
PCManticore
added
reviewed-waiting-updates
and removed
Blocked 🚧
…ead check of the equality oof args type and value
…per_delegation. Thanks @PCManticore.
pylint/checkers/classes.py
Outdated
| return | ||
| if not _has_different_parameters_default_value(meth_node.args, function.args): | ||
| self.add_message('useless-super-delegation', node=function, args=(function.name,)) | ||
| # For each method that have an ancestor, the previous check is enough |
There was a problem hiding this comment.
I don't think this is correct. We still need to check that the call and the definition are equivalent, since this change only helps with default values. Here's an example where your code will emit a useless-super-delegation even though the super() call is not equivalent.
class A:
def test(self, a=None):
pass
class B(A):
def test(self, a=None):
super(B, self).test(a=a+1)
From what I see, if it these two functions have different default values, we should just skip the ancestor. If all ancestors behave the same, bail out and don't check for useless super delegation any longer. Otherwise, go to the signature check
There was a problem hiding this comment.
@PCManticore you are absolutely right. I will correct this ASAP. Thanks.
…e return early if the default values are different. A method redefinition is really useless if the default values are the same in the base class and in the child class but also if the call of the method is the same as the definition.
…ld method are the same but the call signature differs from the definition.
…thod defined or not in intermediate class
…tion and the first ascendant
|
Thank you so much @hippo91 for this contribution! |
4 participants
Deletion of false-positive
useless-super-delegationwhen parameters default values in child class are different from those in the base class.Close #1085
I just created a method that check if the default value of
astroid.Arguments.argsare different in the base method and in the overridden one. I wonder if i also should checkastroid.Arguments.kwargandastroid.Arguments.kwonlyargs.Moreover @AWhetter suggested this bug could be linked to #1553 but i haven't been able to solve it and #1085 together.