-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Turn TextIOWrapper(buffer) into a protocol #11420
Conversation
4321cc2
to
9ea524b
Compare
This could also use some regression tests, but I'm currently a bit short on time. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Why do we need all those empty |
Because otherwise mypy gets confused about |
Ugh. Yeah, renaming seems the easiest option there for now... thanks! |
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Alex Waygood <[email protected]>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hmm, looks like we've been here before: The differences that PR has to this PR are:
|
Hrm, cc @JelleZijlstra. Since this PR seem to fix the issues without adding to the primer output. I suggest to merge this first, and then have another look at Jelle's old PR. |
That sounds reasonable to me, though if it would be more accurate to have the |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for picking this up. The major remaining difference from my PR seems to be that you're not overriding buffer
and detach
. This helps to get rid of some of the primer hits.
def name(self) -> Any: ... | ||
@property | ||
def closed(self) -> bool: ... | ||
def read(self, size: int = ..., /) -> ReadableBuffer: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def read(self, size: int = ..., /) -> ReadableBuffer: ... | |
def read(self, size: int, /) -> ReadableBuffer: ... |
It always calls read with an argument. Note this change makes the protocol more permissive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately that's not true: https://github.com/python/cpython/blob/81e140d10b77f0a41a5581412e3f3471cc77981f/Modules/_io/textio.c#L1977.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment linking to that line in the source code?
Diff from mypy_primer, showing the effect of this PR on open source code: streamlit (https://github.com/streamlit/streamlit)
- lib/tests/streamlit/web/server/component_request_handler_test.py:176:38: error: Argument 1 to "TextIOWrapper" has incompatible type "str"; expected "IO[bytes]" [arg-type]
+ lib/tests/streamlit/web/server/component_request_handler_test.py:176:38: error: Argument 1 to "TextIOWrapper" has incompatible type "str"; expected "_WrappedBuffer" [arg-type]
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. #11420 (comment) is optional.
I think this also fixes #6061? |
Closes: #11418