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

Move no-self-use to optional checker #5502

Closed
cdce8p opened this issue Dec 11, 2021 · 3 comments · Fixed by #6448
Closed

Move no-self-use to optional checker #5502

cdce8p opened this issue Dec 11, 2021 · 3 comments · Fixed by #6448
Labels
Checkers Related to a checker Enhancement ✨ Improvement to a component
Milestone

Comments

@cdce8p
Copy link
Member

cdce8p commented Dec 11, 2021

Related #5496

I recently came across code like this

class Parent:
    @staticmethod
    def get_email() -> str:
        return "test@test.com"

class Child(Parent):
    def __init__(self, email: str) -> None:
        self.email = email

    def get_email(self) -> str:
        return self.email


def func(x: Parent):
    print(x.get_email())

Although get_email is only ever called on an instance, the author choose to add the staticmethod decorator. I believe, just so pylint wouldn't complain about no-self-use.

With #5412 this will however now emit a arguments-differ warning on Child.get_email (with and without staticmethod).

Proposal
I would like to propose moving no-self-use to an optional checker.

Reason
Although this check might be helpful in some instances, usually not using self is no indication that a method should be static. As shown in the example, sometimes this leads to devs writing worse code just to satisfy pylint instead of adding pylint: disable.

@cdce8p cdce8p added Checkers Related to a checker Discussion 🤔 labels Dec 11, 2021
@Pierre-Sassoulas
Copy link
Member

Totally agree, I'm always disabling it myself. But isn't this issue a small subpart of #3512 though ? (On mobile so I added the number by memory, the issue might not be the right one, it should be disabling some message by default) AWhetter made a list of what we should do for all messages basically, I would appreciate your opinion on this.

@derula
Copy link

derula commented Apr 23, 2022

Hm, I wonder, instead of disabling it, would it make sense to inspect the entire class hierarchy? For example:

  • If self is not used in this method, check the parent class.
  • If self is not used in the parent class, check its other subclasses.
  • Only if no other "sibling" class uses it, mark it as "no self use."

Why? I want to be reminded if neither version of a method uses the self argument. So I don't just want to disable this check in general. It's just this case where another related class does use it that it's counter-productive.

But I guess if that's too complicated or comes with a million other caveats, disabling it wouldn't be a huge loss.

@cdce8p
Copy link
Member Author

cdce8p commented Apr 23, 2022

@derula Unfortunately, we can only check parent / base classes since there are no references to possible child or sibling classes. Usually however, it's a method in parent class for which the no-self-use warning is emitted.

All in all I think the added value is quite minimal in which case it's more important to prevent false-positives than false-negatives. Especially when the false-positives lead to undesirable behavior by the user just to avoid the warning (marking an instance method as static).

Moving the check to an optional checker would allow anyone who wants to, to continue using it.

@Pierre-Sassoulas Pierre-Sassoulas added Enhancement ✨ Improvement to a component and removed Discussion 🤔 labels Apr 27, 2022
denpamusic added a commit to denpamusic/PyPlumIO that referenced this issue Jun 1, 2022
Rainyan added a commit to Rainyan/discord-bot-ntpug that referenced this issue Jun 14, 2022
The "no-self-use" Pylint checker has been moved to optional extension
by pylint-dev/pylint#5502.
LefterisJP added a commit to LefterisJP/rotkehlchen that referenced this issue Feb 7, 2023
It's been moved to optional checker and we won't activate it. Was not
really useful and switching to static methods in all places would
carry other interesting problems and no positives as shown in: pylint-dev/pylint#5502
LefterisJP added a commit to rotki/rotki that referenced this issue Feb 7, 2023
It's been moved to optional checker and we won't activate it. Was not
really useful and switching to static methods in all places would
carry other interesting problems and no positives as shown in: pylint-dev/pylint#5502
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Checkers Related to a checker Enhancement ✨ Improvement to a component
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants