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 <Alex.Waygood@Gmail.com>
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.
JelleZijlstra
left a comment
There was a problem hiding this comment.
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.
| 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.
Unfortunately that's not true: https://github.com/python/cpython/blob/81e140d10b77f0a41a5581412e3f3471cc77981f/Modules/_io/textio.c#L1977.
There was a problem hiding this comment.
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]
|
AlexWaygood
left a comment
There was a problem hiding this comment.
LGTM. #11420 (comment) is optional.
|
I think this also fixes #6061? |
Closes: #11418