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.

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

Allow hasattr() function #2228

Closed
Dresdn opened this issue Oct 22, 2021 · 7 comments · Fixed by #3232
Closed

Allow hasattr() function #2228

Dresdn opened this issue Oct 22, 2021 · 7 comments · Fixed by #3232
Labels
bug Something isn't working

Comments

@Dresdn
Copy link

Dresdn commented Oct 22, 2021

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 Dresdn added the bug Something isn't working label Oct 22, 2021
@sobolevn
Copy link
Member

@Dresdn
Copy link
Author

Dresdn commented Oct 22, 2021

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

Dresdn commented Oct 27, 2021

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

@sobolevn
Copy link
Member

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

Dresdn commented Jan 31, 2022

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

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

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants