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

Implement flake8-async #4246

Closed
dacevedo12 opened this issue May 5, 2023 · 11 comments · Fixed by #4432
Closed

Implement flake8-async #4246

dacevedo12 opened this issue May 5, 2023 · 11 comments · Fixed by #4432
Labels
good first issue Good for newcomers plugin Implementing a known but unsupported plugin

Comments

@dacevedo12
Copy link

https://github.com/cooperlees/flake8-async

It'd be good to implement (and hopefully extend) the concept of this project in enforcing best practices for asyncio usage.

@charliermarsh charliermarsh added plugin Implementing a known but unsupported plugin good first issue Good for newcomers labels May 5, 2023
@charliermarsh
Copy link
Member

Looks pretty straightforward for anyone that's interested in learning to write rules for Ruff.

@inikolaev
Copy link

I want to give it a try - have a working prototype already and will push shortly.

@qdegraaf
Copy link
Contributor

I want to give it a try - have a working prototype already and will push shortly.

Missed this. Had some chats about working on this the last few days in the Discord but did not mention it here. Just opened PR, but as a first time contributor I might not have come up with the most elegant solution. Happy to close for your solution if it's better, or merge the two or something else along those lines.

@inikolaev
Copy link

I want to give it a try - have a working prototype already and will push shortly.

Missed this. Had some chats about working on this the last few days in the Discord but did not mention it here. Just opened PR, but as a first time contributor I might not have come up with the most elegant solution. Happy to close for your solution if it's better, or merge the two or something else along those lines.

I'm a first time contributor as well, feel free to peek at what I've done so far: #4433

Still missing some docs for the violations

@inikolaev
Copy link

I should have checked open PRs as well, but assumed nobody was working on it yet :)

@qdegraaf
Copy link
Contributor

Yeah I could have been more clear as well 😅 . Going to leave it to @charliermarsh to decide what's to happen for this one and be a bit more transparent on the next one.

@dacevedo12
Copy link
Author

Please keep in mind the original one has a bug.

time.sleep -> triggers rule
from time import sleep -> doesnt

It also lacks validations for async defs that dont use await inside them.

Just pointing this out if the PRs port the code exactly as it is on that project to avoid the same mistakes

@dacevedo12
Copy link
Author

Thanks for the contributions. I'd like to complement those myself but my skills in rust are currently lacking

@qdegraaf
Copy link
Contributor

Please keep in mind the original one has a bug.

time.sleep -> triggers rule from time import sleep -> doesnt

It also lacks validations for async defs that dont use await inside them.

Just pointing this out if the PRs port the code exactly as it is on that project to avoid the same mistakes

Good catch., cheers! I'll mark mine as draft with some TODOs, but will hold off on implementing them in case #4433 covers that already, in which case I can close mine.

@charliermarsh
Copy link
Member

I'm going to move forward with reviewing #4432 since it strikes me as a bit more complete (uses scoping logic to detect the async function context, includes documentation for the rules). I'm sorry @inikolaev, I know it's disappointing to put in work on a PR and not see it merged into the codebase, but we now have two similar PRs for the same scope of the work and so I have to choose between them somehow.

If you're interested in contributing, I'd love to help you find something else to work on (e.g., I'd love to finish the flake8-pyi rules from #848), but I totally understand if you'd rather not.

@inikolaev
Copy link

I'm going to move forward with reviewing #4432 since it strikes me as a bit more complete (uses scoping logic to detect the async function context, includes documentation for the rules). I'm sorry @inikolaev, I know it's disappointing to put in work on a PR and not see it merged into the codebase, but we now have two similar PRs for the same scope of the work and so I have to choose between them somehow.

If you're interested in contributing, I'd love to help you find something else to work on (e.g., I'd love to finish the flake8-pyi rules from #848), but I totally understand if you'd rather not.

No worries, I will look for something else to implement 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers plugin Implementing a known but unsupported plugin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants