Re-code flake8-trio and flake8-async rules to match upstream #10416
Conversation
|
This is mostly done, except for the previous ASYNC101 and ASYNC102 which don't exactly match any the of the new ASYNC rules. The closest matches are ASYNC220 and ASYNC222, but the behaviors don't match exactly. Part of the problem is that the previous rules should be split into multiple rules (this is easy). The other problem which I'm not sure how to resolve is that it doesn't seem like any of the new ASYNC rules check for |
|
|
Ok so I modified the implementation of ASYNC220 and ASYNC222, as well as added ASYNC221 so that the three rules would match the behavior of flake8-async. For now I added a check for time.sleep as part of ASYNC222. |
|
Oh, I'd missed that we don't check for time.sleep anymore. It's a bad match for ASYNC222, as the recommended replacement is using |
augustelalande
force-pushed
the
merge-trio-async
branch
from
66f14b4
to
bcb1947
Compare
|
Ok I moved the time.sleep check to a newly created ASYNC251 |
|
We've just released ASYNC251 upstream, so this is back to matching the merged flake8-async plugin. |
|
Note that we'll need to add "redirects" for all of these rules, see rule_redirects.rs. See #9756 for example. We may be able to make use of #9691 as well, not sure yet. |
|
Ok I added redirects for the old TRIO rules. But as mentioned the old ASYNC rules don't directly correspond to new ASYNC rules. |
| ("TRIO109", "ASYNC109"), | ||
| ("TRIO11", "ASYNC11"), | ||
| ("TRIO110", "ASYNC110"), | ||
| ("TRIO115", "ASYNC115"), |
There was a problem hiding this comment.
It looks like some ASYNC rules were also moved from one code to another.
There was a problem hiding this comment.
ASYNC100 moved to ASYNC210, (but the ASYNC100 code was taken over by TRIO100)
ASYNC101 was split into ASYNC220, ASYNC221, ASYNC222, ASYNC230, and ASYNC251
ASYNC102 was merged into ASYNC220 and ASYNC221
There was a problem hiding this comment.
@charliermarsh any advice on this?
There was a problem hiding this comment.
I think what you have here is right... The TRIO rules are gone, so lets redirect them away. But the ASYNC rules probably can't be safely redirected.
crates/ruff_linter/src/codes.rs
Outdated
| (Flake8Async, "221") => (RuleGroup::Preview, rules::flake8_async::rules::RunProcessInAsyncFunction), | ||
| (Flake8Async, "222") => (RuleGroup::Preview, rules::flake8_async::rules::WaitForProcessInAsyncFunction), | ||
| (Flake8Async, "230") => (RuleGroup::Preview, rules::flake8_async::rules::BlockingOpenCallInAsyncFunction), | ||
| (Flake8Async, "251") => (RuleGroup::Preview, rules::flake8_async::rules::BlockingSleepInAsyncFunction), |
There was a problem hiding this comment.
I think these probably shouldn't be in preview.
|
I've got some comments on the human-friendly rule names you have, and it might be good if we synced them to ease transition between flake8-async and ruff. See python-trio/flake8-async#248 (comment) (I'm sorry we're moving so quickly as of late 😁) |
|
This PR has some merge conflicts now, anything I can do to help? I'm motivated to add (the new) ASYNC101 to Ruff because of the problems mentioned in the draft PEP 789. |
|
The main issue is that it's going to be a breaking change for users (e.g., |
|
@zanieb - let's include this in v0.5.0 since it's starting to cause problems. |
|
@augustelalande -- I'm happy to take responsibility for resolving any conflicts. |
|
@charliermarsh I'll do it now. I'm also gonna update the rules with the new names from python-trio/flake8-async#248 (comment) |
|
Nevermind the last part. Updating all the rule names is a significant effort so should be a separate PR. |
Summary
Re-code flake8-trio and flake8-async rules to match upstream. Resolves #10303.
Test Plan