CLN: use staticmethods where applicable. #34102
Comments
joooeey
added
Needs Triage
MarcoGorelli
added
Needs Discussion
|
Might also be worth tracking down non-classmethods that dont use |
|
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. |
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 |
|
-1 on any use of staticmethod at all you can refactor to module level or leave |
|
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. |
For this specific example, there are actually some technical reasons for the use of 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 |
Why would they need to be 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? |
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
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 |
|
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 |
My simple search for "@classmethod" says there are only 285 occurrences, and a large part of the ones I randomly looked at are using |
|
That would imply that of the 330 cases, most are instance methods (i. e. normal methods) where By the way a text search of |
Best guess is that you are including results inside generated C files |
|
Yeah, I was only looking at classmethods, not normal methods that don't use For normal methods that don't use self, and only for Now, I personally don't feel very strongly about wether those should be cleaned up to be staticmethods or not. |
|
I agree that this is a lot of work even if it can be automated. I just wanted to document this issue because I noticed it when working on #33821 and we briefly discussed it on that PR. Not sure if it is worth fixing for legacy code. However, I do think that my first bullet point should be implemented, that is, new additions to the code shouldn't include any functions or methods with unused arguments. Lint is only applied on new code, is that right? This might be as easy as changing a single Linter option. I have no idea how our build process works though. (I have to say, it's possible that we're already preventing this. But I don't know how to check if we do.) |
No, the code checks apply to the whole code base.
In many cases these are worth hunting down, but we would need to whitelist a number of cases where we include unused kwargs for e.g. numpy compat |
|
I see. For cases where unused kwargs are required, Pylint's suggested fix of using |
|
Rather than closing, could/should this be repurposed as "delete unused arguments"? |
|
closing as there hasn't been much/any uptake here |
Issue
In my arbitrary sample of three
.pyfiles in the code base, Two files had multiple class methods in which theclsattribute 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
selfandcls) to static methods. Run that script across the Pandas codebase.The text was updated successfully, but these errors were encountered: