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

Expand docs for ASYNC109 #13146

Merged
merged 7 commits into from
Sep 1, 2024
Merged

Conversation

jamesbraza
Copy link
Contributor

Summary

Documenting when to use or not use ASYNC109 a bit more.

Closes #12353

Test Plan

It wasn't tested

Copy link
Contributor

github-actions bot commented Aug 29, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I agree the docs for this function can be improved, and I think this is a step in the right direction overall. However, I'm not sure the PR fully addresses @jakkdl's comments in #12353 (comment) and #12353 (comment). It would be great if the rule's docs could attempt to give a proper explanation of why the rule exists in the first place (to help enforce opinionated design decisions relating to a broader philosophy of how structured concurrency works), probably linking to https://vorpus.org/blog/some-thoughts-on-asynchronous-api-design-in-a-post-asyncawait-world/#timeouts-and-cancellation.

I guess those updates don't necessarily need to be made in this PR, but I don't think this PR is sufficient to close out #12353 as it currently stands

@AlexWaygood AlexWaygood added the documentation Improvements or additions to documentation label Aug 29, 2024
@AlexWaygood AlexWaygood self-assigned this Aug 30, 2024
@jamesbraza
Copy link
Contributor Author

Alright, I just added a small blurb mentioning structured concurrency and linking that blog post 👌

Thanks for the detailed and thoughtful review

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@AlexWaygood AlexWaygood changed the title Expanding ASYNC109 docs Expand docs for ASYNC109 Sep 1, 2024
@AlexWaygood AlexWaygood enabled auto-merge (squash) September 1, 2024 10:12
@AlexWaygood AlexWaygood merged commit 1be8c2e into astral-sh:main Sep 1, 2024
18 checks passed
Copy link

codspeed-hq bot commented Sep 1, 2024

CodSpeed Performance Report

Merging #13146 will degrade performances by 5.48%

Comparing jamesbraza:better-async109-docs (7f61ec7) with main (52d8847)

Summary

❌ 1 regressions
✅ 31 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main jamesbraza:better-async109-docs Change
linter/all-rules[numpy/globals.py] 727.5 µs 769.7 µs -5.48%

@jamesbraza jamesbraza deleted the better-async109-docs branch September 1, 2024 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive ASYNC109 when timeout goes to asyncio.timeout
2 participants