-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
code cleanup after 91x PR #145
Conversation
Actually, since I landed on And with the only remaining user of it being visit_While_test ... and I don't think we care about handling |
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.
Minor nitpicks but great overall, go ahead and merge once you've addressed to your satisfaction 😁
@@ -28,7 +28,7 @@ | |||
|
|||
T = TypeVar("T", bound=Flake8TrioVisitor) | |||
T_CST = TypeVar("T_CST", bound=Flake8TrioVisitor_cst) | |||
T_EITHER = TypeVar("T_EITHER", Flake8TrioVisitor, Flake8TrioVisitor_cst) | |||
T_EITHER = TypeVar("T_EITHER", bound=Flake8TrioVisitor | Flake8TrioVisitor_cst) |
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 think this |
syntax requires Python 3.10?
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 think it's fine due to from __future__ import annotations
?
But regardless, tests run with <3.10, and I don't think we care about whether pyright
running on 3.9 passes.
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.
🤦 it's fine at runtime because we're in an if TYPE_CHECKING:
block and so it never actually runs, nevermind.
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.
haha I didn't realize that either, https://github.com/snok/flake8-type-checking doing it's job!
if isinstance(arg.operator, cst.Minus): | ||
value = -value |
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.
Missing the ~
operator! https://docs.python.org/3/reference/expressions.html#unary-arithmetic-and-bitwise-operations
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.
Good catch! Although I suspect it's quite rare to invert bits in range loops 😅
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 certainly hope so! At large enough scale though, someone is bound to...
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.
And of course this checker is gonna reach that scale 😁
Reading through https://docs.python.org/3/library/stdtypes.html#range it mentioned # length > sys.maxsize
for i in range(27670116110564327421):
await foo()
yield |
Heh, that note is my fault 😂 (Hypothesis is a great way to find edge cases...) |
ooh, nice! |
Fix comments from #143 and general minor code cleanup