-
-
Notifications
You must be signed in to change notification settings - Fork 344
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
Enable ruff's flake8-trio
rule
#2947
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2947 +/- ##
==========================================
+ Coverage 99.44% 99.64% +0.19%
==========================================
Files 117 117
Lines 17564 17565 +1
Branches 3166 3166
==========================================
+ Hits 17467 17503 +36
+ Misses 79 43 -36
- Partials 18 19 +1
|
At the moment the tests will fail until #2946 gets merged, and even then there will be a failure detailed in astral-sh/ruff#9855 that is a bug. |
Disclaimer: I'm the one that wrote 99% of the code in flake8-trio. Ruffs implementation of flake8-trio is only a small set (6 out of ~30) of the full rules: https://github.com/Zac-HD/flake8-trio Although a few of them are handled under other rules. The plugin is intended for users of trio or anyio*, so some rules might not make sense in trio itself, but can ofc then just disable those. It is also possible to run flake8-trio as a standalone*, so we don't have to bring back flake8. (because we wanted to implement autofix with libcst, which isn't possible when running with flake8). But there might be reasons to run it through flake8, I don't fully remember. * yes, both parts of the flake8-trio name are incorrect... python-trio/flake8-async#124 (comment) |
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.
Just quick thing I think isn't great
@@ -165,7 +167,7 @@ def external_thread_fn() -> None: | |||
thread.start() | |||
print("waiting") | |||
while thread.is_alive(): | |||
await sleep(0.01) | |||
await lowlevel.checkpoint() |
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 probably better to use sleep
here because otherwise we'll be busy looping I think?
Like this isn't a matter of "trio just needs another cycle or two of the available tasks" this is "we need to wait for the thread to start and do its stuff"
This happens 2 other places too.
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.
this is also a bug in ruffs implementation, flake8-trio only triggers when the literal is exactly 0
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.
wait no, ruff gave TRIO110, which suggests to await an Event
instead of looping a sleep. Resolving that by replacing with a checkpoint is ... definitely not the way to go.
Maybe I should widen TRIO110 to also catch checkpoints to make sure people don't confuse them and thinks that because it stops warning on checkpoint
that it "solves" it
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 manually changed the 0.02 to a checkpoint because an error was being raised from the sleep there, mostly because the Event part didn't make a lot of sense to me. Should have just done a noqa really.
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.
yeah an Event could plausibly not be possible or make sense in a context, in which case noqa
would be the solution :)
I just want to mention that ruff's TRIO rules are significantly behind |
Given that ruff's version has a few issues and other points mentioned, I don't think this particular rule is fit for Trio at the moment. |
Originally posted by @TeamSpen210 in #2946 (comment)
This pull request enables the
flake8-trio
ruff rule (but in ruff's system it's marked asTRIO
).