pylint icon indicating copy to clipboard operation
pylint copied to clipboard

Check For @singledispatch Decorator in classes without `@staticmethod`

Open cdreimer-thewriter opened this issue 2 years ago • 1 comments

Current problem

The representative code implements single dispatching with the @singledispatch and @staticmethod decorators in a class.

Class Board():

   @singledispatch
   @staticmethod
   def convert_position(position):
        [...]
   
    @convert_position.register(str)
    @staticmethod
    def _(position: str) -> tuple:
        [...]

    @convert_position.register(tuple)
    @staticmethod
    def _(position: tuple) -> str:
        [...]

Pylint reports nothing unusual about using @singledispatch inside a class.

Desired solution

Pylint should check for the @singledispatch decorator inside a class. According to the documentation, the @singledispatchmethod decorator is for class methods.

Additional context

The code works with the @singledispatch decorator as long as the single dispatch methods have the @staticmethod decorator (i.e., stateless methods). If the @staticmethod decorators were removed or replaced with the @classmethod decorator, the code stops working and the resulting error messages will indirectly point out the wrong decorator. A pylint check would prevent that from happening.

cdreimer-thewriter avatar Jun 11 '22 20:06 cdreimer-thewriter

Thanks for the clear report. This could be added to the StdlibChecker. No obligation, but if you have (or anyone reading has) an interest in preparing a patch yourself I'd be glad to answer questions as you go.

jacobtylerwalls avatar Jun 23 '22 03:06 jacobtylerwalls

Hey @jacobtylerwalls - landing here from the hacktoberfest label and not familiar with pylint's codebase. Is this a similar implementation as _check_lru_cache_decorators in StdlibChecker?

ramonsaraiva avatar Sep 30 '22 16:09 ramonsaraiva

@ramonsaraiva That should indeed be relatively similar. In fact, I think we can build upon some of the code that is already there. You can probably also look at https://github.com/PyCQA/pylint/blob/main/tests/functional/m/method_cache_max_size_none.py for some inspiration for functional tests.

DanielNoord avatar Sep 30 '22 18:09 DanielNoord

Awesome, I'll be reading more about how everything works and attempt to come up with a draft patch for this. A few review interactions will most likely be needed. Thanks!

ramonsaraiva avatar Sep 30 '22 19:09 ramonsaraiva

Created that draft PR and added in its description a bit of my thought process and things that I believe can be improved and simplified. I just want to have a better sense if I'm going in the right direction.

I'll be digging more on utilities and built-in functionality that I can use to simplify all of that but I thought it'd be helpful to have a draft available for you to take a look.

ramonsaraiva avatar Oct 05 '22 06:10 ramonsaraiva

I think https://github.com/PyCQA/pylint/commit/4d95d21ad41e1a219cc5917181b102e5de5832bd and https://github.com/PyCQA/pylint/commit/cc64ded7c6c5e5fd053e5a58fbbeebce0cff93c8 bring a much cleaner version that https://github.com/PyCQA/pylint/commit/3dc361624cd805e32972372257e3ee48ef1ac84a had - still open to suggestions.

I added a new util function for singledispatchmethod (similar to the singledispatch one) and moved some reusable logic to a separate one. There was a minor modification to the way we check for register calls, cause now with annotated types register can also be an attribute and not strictly a call.

ramonsaraiva avatar Oct 05 '22 13:10 ramonsaraiva

Manually closing since MR is merged.

mbyrnepr2 avatar Oct 31 '22 19:10 mbyrnepr2