wemake-python-styleguide icon indicating copy to clipboard operation
wemake-python-styleguide copied to clipboard

Allow hasattr() function

Open Dresdn opened this issue 3 years ago • 7 comments

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

Dresdn avatar Oct 22 '21 18:10 Dresdn

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?

sobolevn avatar Oct 22 '21 18:10 sobolevn

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 avatar Oct 22 '21 18:10 Dresdn

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

Dresdn avatar Oct 27 '21 14:10 Dresdn

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.

sobolevn avatar Jan 29 '22 15:01 sobolevn

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?

Dresdn avatar Jan 31 '22 02:01 Dresdn

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

rubancar avatar Jul 20 '22 15:07 rubancar

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?

chasefinch avatar Apr 11 '24 15:04 chasefinch