Uh oh!
There was an error while loading. Please reload this page.
Turn TextIOWrapper(buffer) into a protocol - #11420
Conversation
4321cc2 to
9ea524bComparesrittau
commented
Feb 14, 2024
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 |
srittau
commented
Feb 14, 2024
Because otherwise mypy gets confused about |
AlexWaygood
commented
Feb 14, 2024
Ugh. Yeah, renaming seems the easiest option there for now... thanks! |
This comment has been minimized.
This comment has been minimized.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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.
AlexWaygood
commented
Feb 14, 2024
Hmm, looks like we've been here before: The differences that PR has to this PR are:
|
srittau
commented
Feb 14, 2024
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. |
AlexWaygood
commented
Feb 14, 2024
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.
| defread(self, size: int= ..., /) ->ReadableBuffer: ... | |
| defread(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?
Uh oh!
There was an error while loading. Please reload this page.
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.
AlexWaygood
commented
Feb 14, 2024
I think this also fixes #6061? |
Closes: #11418