CLN: use staticmethods where applicable.

[joooeey]

Issue

In my arbitrary sample of three .py files in the code base, Two files had multiple class methods in which the cls attribute was never used. It's entirely incomprehensible to me why these are specifically decorated as class methods since they don't access the class. It would improve code readability if they were static methods. Technically, most of them could also be module-level functions but refactoring that appears to be way more effort.

An example is pandas/plotting/matplotlib/core.py.

Proposed fix

• To prevent anyone from making this mistake again, we should make sure our Linter rejects methods with unused arguments such as PyLint does.
• Write a script that converts all methods (including class methods) with an unused first argument (typically self and cls) to static methods. Run that script across the Pandas codebase.
• Fix any resulting test failures or other issues. Though very likely there will be none because a static method is virtually indistinguishable from a class method or instance method except if you check the type directly.
• Make the pull request.

[jbrockmendel]

Might also be worth tracking down non-classmethods that dont use self.

[CloseChoice]

I just ran the pycharm code inspection and found 330 "method may be static warnings". Before we can make an automated check, we first need to change all of them. It may make sense to split this into multiple merge requests.

[joooeey]

Might also be worth tracking down non-classmethods that dont use self.

Absolutely. The proposed fix included that from the beginning. The only reason the issue description focuses on the class methods is that I've only seen this issue with class methods in Pandas. We could even expand to address all methods with unused arguments and if it's not self or cls we can del the unused argument at the start of the method as PyLint recommends. This should be simple to automate too.

[jreback]

-1 on any use of staticmethod at all
this is an anti pattern in python

you can refactor to module level or leave

[joooeey]

I understand your distaste for static method although I don't see why it's that extreme. The current state of hundreds of methods that are functionally static methods but not declared as such is what I'm worried about. Refactoring to static method is pragmatic in this case because it can be automated and leaves interfaces intact. But you are definitely free to take on this PR and refactor the ~330 class methods to module level functions. Maybe that can be scripted somehow too, or do it by hand. I agree that that would be cleaner, I just can't see anyone investing the time to do it.

[jorisvandenbossche]

An example is pandas/plotting/matplotlib/core.py.

For this specific example, there are actually some technical reasons for the use of classmethod. For example, the _plot class methods need to be methods on the class, because they can be overrriden by subclasses (so factoring out as module level free-standing function doesn't work here). And second, although many of them don't use cls at all, they still need to be classmethods, because a subclass can use cls (and this actually happens in LinePlot._plot).
I only looked at _plot, but similar arguments might apply for the other class methods in this file as well.

This is of course just about this specific file. There certainly can be other places where class methods are used less purposefully. But so although we can talk about the general principle of not using class methods if it is not needed (with which I agree), it might be useful/needed in the end to check case by case.

But for example I check some random other classmethod occurences, and they were almost all valid use cases AFAIK.

[joooeey]

And second, although many of them don't use cls at all, they still need to be classmethods, because a subclass can use cls (and this actually happens in LinePlot._plot).

Why would they need to be classmethod? They should be staticmethod. Derived classes can still override it with a classmethod. No? Am I missing something?

I was aware that overriding might be a concern and that's a big reason why I suggested the specific proposed fix in the first post. Thanks for spelling it out! So why don't we focus on the fix I proposed (the low hanging fruit) and then consider refactoring static methods in the code base to module level where that makes sense?

[jorisvandenbossche]

Derived classes can still override it with a classmethod. No? Am I missing something?

I suppose so yes, but personally I wouldn't find that better practice to overide a staticmethod as a classmethod, than to have some classmethods that don't use cls (but that's just a personal opinion).

So why don't we focus on the fix I proposed (the low hanging fruit)

I am personally fine with converting some classmethods to staticmethods, but I am still wondering if there are really that much cases if excluding the cases like the matplotlib file (eg in pandas/core/generic.py, there are 8 classmethod, all of which use cls. But I suppose it might vary a lot by file? And I am by accident looking in the "wrong" files)

[joooeey]

I don't know where they are either. But I guess pycharm would tell you. I maintain that all 330 cases that pycharm reports are bad, simply because unused method (or function) arguments are a code smell. That also applies to the cases where cls is unused but a subclass overrides the method.