Skip to content

trio232 now also handles annotated assignments, and added lots of tests - #111

Merged
Zac-HD merged 1 commit into
python-trio:mainfrom
jakkdl:path_file_objects
Jan 31, 2023
Merged

Zac-HD merged 1 commit into
python-trio:mainfrom
jakkdl:path_file_objects

Conversation

@jakkdl

@jakkdl jakkdl commented Jan 28, 2023

Copy link
Copy Markdown
Member

Any one of these unhandled cases probably isn't too bad to handle - but to handle all of them would end up making this a pretty complex check. See #110
But if you think any in particular is a common pattern, I can implement that.

@jakkdl
jakkdl force-pushed the path_file_objects branch from 76f2c2a to 985f9a2 Compare January 28, 2023 14:15

@Zac-HD Zac-HD left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks great!

Comment on lines +12 to +14
# there might be non-sync calls on TextIOWrappers? - but it will currently trigger
# on all calls
f.anything() # TRIO232: 4, 'anything', 'f'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, this is an interesting point... looking at the IOBase docs there are some sync methods that are still fine even in an async context, e.g. .seekable(), .tell(), .isatty(), etc. (though don't trust my quick skim on the details!).

We might therefore want to further restrict this to a blocklist of known-slow-IO methods, e.g. .read(), .readline(), .seek(), etc. That'd be a future PR though.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm pretty sure open&close are still no-go though, so only way to trigger this would be opening a file in a sync context, sending around the TextIOWrapper as a parameter inside your async stuff, and then close it once back in a sync context. Which seems like a huge stretch if the only thing you care about is seekable/tell/isatty.
But this might definitely be much more relevant with other objects.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fair enough, and I definitely meant the "might"! I agree that this doesn't seem like it would be much of an improvement in practice, so let's not bother.

@Zac-HD
Zac-HD merged commit 637d026 into python-trio:main Jan 31, 2023
@jakkdl jakkdl mentioned this pull request Jan 31, 2023
10 of 12 tasks
@jakkdl
jakkdl deleted the path_file_objects branch March 6, 2023 15:30
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