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

Allow adding parameters in an overridden method, without tripping arguments-differ #1556

Closed
benf-wspdigital opened this issue Jun 26, 2017 · 5 comments · Fixed by #5539
Closed
Labels
C: arguments-differ Issues related to 'arguments-differ' and 'signature-differs' checks Enhancement ✨ Improvement to a component
Milestone

Comments

@benf-wspdigital
Copy link

benf-wspdigital commented Jun 26, 2017

The arguments-differ check will complain about a method which loses arguments that the superclass's method will allow. Such code violates the Liskov Substitution Principle, so IMO should trigger arguments-differ.

What is not desirable, IMO, is to have the same check complain about:

class Ipsum:
    def dolor(self, sit):
        pass


class LoremIpsum(Ipsum):
    def dolor(self, sit, amet=None):
        handle(amet)
        super().dolor(sit)

That code does not violate the LSP; an instance of LoremIpsum will function just fine as an instance of Ipsum. Its methods can be called in the same way as methods of Ipsum.

How can I have PyLint not complain about code like this which AFAICT does not violate LSP, while still correctly flagging LSP violations?

@PCManticore
Copy link
Contributor

My view on this issue is that people could implement a different method that does the right thing with the default parameter, rather than overloading this method with a parameter that doesn't have a place in the base classes.
But since I'm noticing a couple of new issues created just for this problem, I guess people don't necessarily want to do that, so I think we could change back this checker to accept optional and variadic parameters as it was doing previously.

@PCManticore PCManticore added the Enhancement ✨ Improvement to a component label Jun 26, 2017
@benf-wspdigital
Copy link
Author

benf-wspdigital commented Jun 27, 2017

Thank you for considering the use cases.

My view on this issue is that people could implement a different method that does the right thing with the default parameter

That IMO should be a separate check. I can see that people might want it, but LSP does not mandate what you describe there.

I guess people don't necessarily want to do that, so I think we could change back this checker to accept optional and variadic parameters as it was doing previously.

Perhaps arguments-differ is too broad a name?

Maybe two separate checks:

  • For the LSP violation, arguments-incompatible-with-superclass-method (arguments from the superclass method are missing; arguments required that are not in the superclass method; other incompatibilities?)

  • For the case you're wanting checked, arguments-added-in-override-method. Method signatures that are entirely compatible with the superclass method would not fail this check.

@PCManticore
Copy link
Contributor

Apart of the names, which are a bit too long, this would make sense.

@rogalski
Copy link
Contributor

Maybe slightly shorter: args-conflict-with-superclass-method, args-added-in-overridden-method.

@jnsnow
Copy link
Contributor

jnsnow commented Sep 6, 2019

Is there a reason the extension of parameters is considered an anti-pattern by pylint?

Specifically, I am thinking about things like auxiliary initializers:

@classmethod
def from_foo(cls, arg1):
    tmp = cls()
    tmp.do_something(arg1)
    return tmp

Most often used for providing additional signatures for __init__ when it offers more implementation clarity.

A derived class might want to add some extra parameter while re-using the name, to give the appearance of an overloaded method:

@classmethod
def from_foo(cls, arg1, arg2=None):
    tmp = super(myclass, cls).from_foo(arg1)
    if arg:
        tmp.do_something_else(arg2)
    return tmp

Now, it is indeed possible to right another function entirely that "does the right thing", but if it is conceived strictly as a superset of behavior of the other function, why should we pollute the namespace with a new function name?

(Especially when pylint begins to bark when you have too many public functions.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: arguments-differ Issues related to 'arguments-differ' and 'signature-differs' checks Enhancement ✨ Improvement to a component
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants