-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Improve conditionals #10581
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
Improve conditionals #10581
Conversation
for stmt in node.nodes_of_class(nodes.Call): | ||
expr = stmt.func | ||
if not isinstance(expr, nodes.Attribute) or expr.attrname != "__init__": | ||
if not (isinstance(expr, nodes.Attribute) and expr.attrname == "__init__"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think these changes here are a net improvement, but I'm curious if that's only my opinion or if others think so too.
Though it might be a stretch goal, we could think about if it makes sense to add a checker for these. Haven't looked into it if it would even be practical or if there are just too many conditions to check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to write them just as I'd write the match case, e.g.
nodes.Attribute(attrname = "__init__")
--
If inline match expressions ever get added, this could be
if not (expr match nodes.Attribute(attrname = "__init__")): ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kinda like the final result more, but if the fix is not automated I'm not sure I'm going to bother.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10581 +/- ##
==========================================
+ Coverage 95.93% 95.95% +0.02%
==========================================
Files 176 176
Lines 19471 19456 -15
==========================================
- Hits 18679 18669 -10
+ Misses 792 787 -5
🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
for stmt in node.nodes_of_class(nodes.Call): | ||
expr = stmt.func | ||
if not isinstance(expr, nodes.Attribute) or expr.attrname != "__init__": | ||
if not (isinstance(expr, nodes.Attribute) and expr.attrname == "__init__"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kinda like the final result more, but if the fix is not automated I'm not sure I'm going to bother.
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉 This comment was generated for commit f3701d7 |
One of the remains from my match PRs. One of the great things about match IMO is that it forces the reader to think about the conditional in a quite linear way. Its limitations, no
not
andand
, make it more readable in some cases.These are some if conditionals which weren't quite enough for match, but updating them might still be worth the effort. E.g.