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

ASYNC102 no longer raises warning for calls to .aclose() on objects #222

Merged
merged 5 commits into from
Mar 20, 2024

Conversation

jakkdl
Copy link
Member

@jakkdl jakkdl commented Mar 19, 2024

See #156

I'm not sure if #156 also covers more general discussion on if/how to overhaul async102 and should remain open after this is merged.

@Zac-HD
Copy link
Member

Zac-HD commented Mar 19, 2024

Very quick notes:

  • we should allow this in an except clause too
  • only exempt if the call takes no arguments, i.e. still error on await x.aclose(foo)
  • don't exempt asyncio-only code; this is a trio/anyio semantics thing
  • merge conflict from the other PR 😅

and then I think we're ready and it'll close the linked issue.

@jakkdl
Copy link
Member Author

jakkdl commented Mar 20, 2024

don't exempt asyncio-only code; this is a trio/anyio semantics thing

I interpreted this as "if running with --anyio and not importing anyio/trio, or only importing asyncio => don't mark as safe; but if running with --anyio and/or importing anyio/trio and importing asyncio => mark as safe", so if you have a mixed project it'll lean towards not reporting acloses. But I could make it more aggressive and warn if there's any risk that this part concerns asyncio-code (i.e. asyncio in self.library -> "running with --asyncio; or asyncio is imported")

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Nice! Thanks, @jakkdl 🙂

@Zac-HD Zac-HD merged commit 8719f3f into python-trio:main Mar 20, 2024
9 checks passed
@jakkdl jakkdl deleted the async102_aclose branch March 22, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants