wemake-services / wemake-python-styleguide

The strictest and most opinionated python linter ever!
https://wemake-python-styleguide.rtfd.io
MIT License
2.51k stars 378 forks source link

Allow hasattr() function #2228

Open Dresdn opened 3 years ago

Dresdn commented 3 years ago

What's wrong

WPS421 forbids calling some built-in functions, which includes hasattr().

How it should be

The function hasattr() should be allowed. There are certain scenarios where using hasattr() makes sense, and sometimes is even faster (as even outlined in the referenced video @ 13:34-14:15 mark)

For code readability, if hasattr(foo, 'bar'): is cleaner than if getattr(foo, 'bar', None):.

I believe there was an argument against using hasattr() in python2, but I don't believe the same argument is valid for python3.

Flake8 version and plugins

n/a

pip information

n/a

OS information

n/a

sobolevn commented 3 years ago

Your link to a video is https://wemake-python-stylegui.de/en/latest/pages/usage/violations/best_practices.html#wemake_python_styleguide.violations.best_practices.WrongFunctionCallViolation

Can you please share a correct one?

Dresdn commented 3 years ago

Oops, my wording was awful. I meant to say that the video referenced in the See Also documentation for WPS421 ...

The link to the video is https://www.youtube.com/watch?v=YjHsOrOOSuI

Dresdn commented 2 years ago

I don't mind submitting a PR, but before going through the motions, any thoughts around a yay or nay on this?

sobolevn commented 2 years ago

I confirm that this no longer produce unexpected result:

class A:
    @property
    def x(self):
        print('wow')

hasattr(A, 'x')  # True

x is not evaluated, when x is a @property.

But, when x is a descriptor it still does this ugly thing:

class GetSet:
    def __get__(self, *args):
        print('wow')

class A:
    x = GetSet()

hasattr(A, 'x')  # prints "wow"

Why is that important? Because semantically hasattr is not very expected to run this code. But, getattr has clear semantics: get me this argument and run all the machinery ⚙️ 🔥

So, I am still not completely sold on allowing it.

Dresdn commented 2 years ago

These are good points. However, getattr outputs wow just like hasattr does. By that logic, getattr should be added to the FUNCTIONS_BLACKLIST as well then, yes?

To be clear, I don't advocate blacklisting getattr as there are semantic and performance reasons for using it.

Semantically, I think that if hasattr(A, 'foo') is clearer than if getattr(A, 'foo', False), which was the reasoning behind opening this issue. Do you agree?

rubancar commented 2 years ago

what @Dresdn says makes sense, one more reason in favor of allow hasattr is that the logic behind this one is performed by calling getattr and catching AttributteError, so, at the end both funcions are base on the same logic, It does not make sense to allow getattr and and issue a warning on hasattr

chasefinch commented 6 months ago

I feel that the existing alternative to hasattr is also prone to mistakes, due to the need for a sentinel (None).

Using getattr(item, 'x', None) to see if x has been defined will be False when item.x = None. There isn't another language construct that I know of that can properly tell whether an attr has been defined. And this mistake is more likely, no? It has bitten me before.

In light of this, couldn't hasattr be allowed while we pursue a fix of the descriptor behavior in the Python interpreter itself?