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.

Sign up for GitHub

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

Jump to bottom

Check For @singledispatch Decorator in classes without @staticmethod #6917

Closed
cdreimer-thewriter opened this issue Jun 11, 2022 · 7 comments · Fixed by #7575
Closed

Check For @singledispatch Decorator in classes without @staticmethod #6917

cdreimer-thewriter opened this issue Jun 11, 2022 · 7 comments · Fixed by #7575
Labels
Enhancement ✨ Improvement to a component Good first issue Friendly and approachable by new contributors Hacktoberfest Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Milestone
2.16.0

Comments

@cdreimer-thewriter
Copy link

cdreimer-thewriter commented Jun 11, 2022 •
edited

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 cdreimer-thewriter added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Jun 11, 2022
@jacobtylerwalls
Copy link
Member

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 jacobtylerwalls added Enhancement ✨ Improvement to a component Needs PR This issue is accepted, sufficiently specified and now needs an implementation Good first issue Friendly and approachable by new contributors and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Jun 23, 2022
@jacobtylerwalls jacobtylerwalls changed the title Check For @singledispatch Decorator In A Class Check For @singledispatch Decorator in classes without @staticmethod Jun 23, 2022
@ramonsaraiva
Copy link
Contributor

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?

@DanielNoord
Copy link
Collaborator

@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.

@ramonsaraiva
Copy link
Contributor

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 ramonsaraiva mentioned this issue Oct 5, 2022
Merged
@ramonsaraiva
Copy link
Contributor

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
Copy link
Contributor

I think 4d95d21 and cc64ded bring a much cleaner version that 3dc3616 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.

@mbyrnepr2 mbyrnepr2 added this to the 2.16.0 milestone Oct 31, 2022
@mbyrnepr2
Copy link
Member

Manually closing since MR is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Enhancement ✨ Improvement to a component Good first issue Friendly and approachable by new contributors Hacktoberfest Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
None yet
Milestone
2.16.0
Development

Successfully merging a pull request may close this issue.

@singledispatch and @singledispatchmethod checks for methods and functions ramonsaraiva/pylint
6 participants
@Pierre-Sassoulas @ramonsaraiva @DanielNoord @mbyrnepr2 @jacobtylerwalls @cdreimer-thewriter