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

Add async stdio files #154

Merged
merged 3 commits into from
Dec 21, 2022
Merged

Add async stdio files #154

merged 3 commits into from
Dec 21, 2022

Conversation

rhelmot
Copy link
Contributor

@rhelmot rhelmot commented Dec 8, 2022

Closes #153
Closes #131

This adds stdin, stdout, stderr, and their binary counterparts to aiofiles. It does this by introducing two concepts:

  • that a file can be unattached from a loop and will instead look up the current loop for every operation. The performance impact of this is offset by the fact that for all previously supported uses of aiofiles, the only impact is a single is None check.
  • that a file reference can be indirect, meaning it calls a getter to look it up for every operation. The performance impact of this is isolated to a new base class AsyncIndirectBase. This is only used when opted-in explicitly, which we do for the stdio files in order to support monkeypatching, as is common in the python ecosystem.

@rhelmot
Copy link
Contributor Author

rhelmot commented Dec 8, 2022

There are a handful of superfluous changes in the diff because I ran black on the whole project to format my code.

@Tinche Tinche self-requested a review December 10, 2022 23:48
Copy link
Owner

@Tinche Tinche left a comment

Choose a reason for hiding this comment

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

LGTM, please add a history snippet to the README.

@Tinche Tinche self-requested a review December 15, 2022 10:16

try:
stdin = wrap(sys.stdin, indirect=lambda: sys.stdin)
except TypeError:
Copy link
Owner

Choose a reason for hiding this comment

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

Where do the TypeErrors come from btw?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sys.stdin may have been patched to be an unusable base type prior to importing aiofiles. This happens specifically when using pytest. I figured I would add it to the rest of the stdio files just be safe.

Copy link
Owner

Choose a reason for hiding this comment

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

I see, alright. Do we want to document this for people somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

imo the best documentation would be replacing the stdin = None with a custom class which raises a descriptive error when you try to use any of its methods. I was hoping I could convince you to do that 😅

lmk if you would like to do that or just add a note to the readme/docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never mind, just thought of a much better solution. One sec.

@rhelmot
Copy link
Contributor Author

rhelmot commented Dec 16, 2022

Okay. new commit. This one makes it so that wrap() no longer has the option to do indirect wrapping, which makes sense - you need to specify the type of the base class if the underlying file (and its type!) can change - you need to be accountable for the contract that the interface you're wrapping provides, which may be different than its stated type.

As a result, things are simplified drastically, and e.g. trying to use stdin from tests yields pytest's error with a meaningful message.

@Tinche
Copy link
Owner

Tinche commented Dec 17, 2022

Cool, thanks. Will merge it in soon.

@Tinche Tinche merged commit ee14e26 into Tinche:main Dec 21, 2022
@pisto
Copy link

pisto commented Feb 7, 2023

Can you please release this on pypi?

@Tinche
Copy link
Owner

Tinche commented Feb 9, 2023

Released!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants