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

Publicly expose AsyncIOWrapper #2864

Closed
wants to merge 15 commits into from

Conversation

CoolCat467
Copy link
Member

@CoolCat467 CoolCat467 commented Nov 5, 2023

This PR exposes _file_io.AsyncIOWrapper to the public.
I was working on a project of mine that has to use trio.wrap_file and calls a few functions that needed to take the returned AsyncIOWrapper object as a parameter, but when typing arguments for said functions I found that AsyncIOWrapper is not exposed publicly and the only way to type it properly would be to manually reach in to trio._file_io and get it.

@CoolCat467 CoolCat467 changed the title Publically expose AsyncIOWrapper Publicly expose AsyncIOWrapper Nov 5, 2023
Copy link

codecov bot commented Nov 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (05c5df2) 99.64% compared to head (f8f0018) 99.64%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2864   +/-   ##
=======================================
  Coverage   99.64%   99.64%           
=======================================
  Files         116      116           
  Lines       17503    17504    +1     
  Branches     3148     3149    +1     
=======================================
+ Hits        17441    17442    +1     
  Misses         43       43           
  Partials       19       19           
Files Coverage Δ
src/trio/__init__.py 100.00% <100.00%> (ø)
src/trio/_file_io.py 100.00% <100.00%> (ø)

@CoolCat467 CoolCat467 requested review from A5rocks and jakkdl November 5, 2023 05:33
@A5rocks A5rocks added the typing Adding static types to trio's interface label Nov 5, 2023
Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

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

Looks good to me at least.

@CoolCat467
Copy link
Member Author

How do I fix the errors test_static_tool_sees_class_members has? The big blob it prints out is of no help on resolving whatever the issue is and I keep getting confused when I encounter these sorts of issues and what to even do to attempt to fix said reported issues.

@A5rocks
Copy link
Contributor

A5rocks commented Nov 6, 2023

Probably similar to what you did for the dir thing!

@TeamSpen210
Copy link
Contributor

Would it be a good idea to expose this? The way it works is kinda complicated, and I imagine we might need to change those hints at some point - exposing it means we're saying it's a more stable API. Also users using it as a type hint means they don't accept any "native" file classes. Like the synchronous IO, it'd be better to define a protocol at the place you use it, with the methods you need.

@CoolCat467
Copy link
Member Author

Would it be a good idea to expose this? The way it works is kinda complicated, and I imagine we might need to change those hints at some point - exposing it means we're saying it's a more stable API. Also users using it as a type hint means they don't accept any "native" file classes. Like the synchronous IO, it'd be better to define a protocol at the place you use it, with the methods you need.

Did you read my usage case? I am trying to specifically handle asynchronous file objects in a function's arguments, and I have to resort to reaching into the internals of trio to type it properly, which I think is unacceptable. I am not intending on handling native synchronous file IO at all, only the async version.

@TeamSpen210
Copy link
Contributor

Yep, what I mean is that you should define a protocol, only include the methods you need, then annotate the function as accepting that. Then a class that doesn't inherit from the wrapper but does define appropriate methods will work.

@CoolCat467
Copy link
Member Author

Probably similar to what you did for the dir thing!

I'm blanking on this, not remembering what you are referring to by this

@A5rocks
Copy link
Contributor

A5rocks commented Nov 18, 2023

Sorry, I just misread a0f58ea

@A5rocks A5rocks self-requested a review November 18, 2023 06:38
@CoolCat467 CoolCat467 marked this pull request as draft January 26, 2024 03:09
@CoolCat467 CoolCat467 closed this Feb 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typing Adding static types to trio's interface user happiness
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants