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

[New Rule] Replace hasattr if else with getattr when appropiate #180

Open
Skylion007 opened this issue May 1, 2023 · 1 comment
Open
Assignees
Labels
enhancement New feature or request

Comments

@Skylion007
Copy link
Contributor

Skylion007 commented May 1, 2023

Explanation

This code is more succinct, readable, and slightly more efficient. We recently opened a few PRs to add this to the PyTorch codebase. Exhibit A: pytorch/pytorch#100360 If you are aware of a different flake8-plugin / ruleset that implements this rule, feel free to recommend it.

Example

# Bad
...  # your bad code goes here
X.Y if hasattr(X, "Y") else Z

# Good
...  # your improved code goes here
getattr(X, "Y", Z)
@Skylion007 Skylion007 added the enhancement New feature or request label May 1, 2023
@r-barnes
Copy link

r-barnes commented May 3, 2023

Note that this is not always more efficient!

The pattern

X.Y if hasattr(X, "Y") else expensive_function()

doesn't evaluate expensive_function(), so it is preferable to

getattr(X, "Y", expensive_function())

This is also bad in the case where we have:

X.mutually_exclusive_1 if hasattr(X, "mutually_exclusive_1") else X.mutually_exclusive_2

Granted: having the interface of a class change like this is bad coding, but it does happen.

In cases where the Z is a constant of some sort, then getattr seems like a slam-dunk for readability in my opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants