Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.9k
typing: accept buffers in typing.IO.write#9084
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
JelleZijlstra commentedNov 4, 2022
The mypy failures are a bug,python/mypy#14002. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
srittau commentedNov 4, 2022
This looks quite disruptive to me and I'm not sure whether it's worth it, considering the slightly legacy nature of |
JelleZijlstra commentedNov 4, 2022
Worth noting that most of the mypy-primer fallout is due to a mypy bug (python/mypy#14002). |
hauntsaninja commentedNov 5, 2022
This seems useful, but given that I think I have a mypy fix, let's wait until it's released:python/mypy#14017 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
AlexWaygood left a comment
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.
Looks like we can go back to this now, with the latest mypy release!
stdlib/codecs.pyi Outdated
| def__iter__(self:Self)->Self: ... | ||
| defwrite(self,data:str)->None: ...# type: ignore[override] | ||
| defwritelines(self,list:Iterable[str])->None: ... | ||
| defwritelines(self,list:Iterable[str])->None: ...# type: ignore[override] # https://github.com/python/mypy/issues/14002 |
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.
We probably don't need thistype: ignore anymore (as well as several others), with the latest mypy release.
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.
Cleaned up all comments referencing that issue.
stdlib/http/client.pyi Outdated
| defparse_headers(fp:io.BufferedIOBase,_class:Callable[[],email.message.Message]= ...)->HTTPMessage: ... | ||
| classHTTPResponse(io.BufferedIOBase,BinaryIO): | ||
| classHTTPResponse(io.BufferedIOBase,BinaryIO):# type: ignore[misc] # https://github.com/python/mypy/issues/14002 |
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.
We do still need thistype: ignore, but not because of the mypy issue linked (which is now closed) -- we need it because the definitions ofwritelines() in the two base classes are incompatible. The same goes for all thetype: ignores inio.pyi and the one inlzma.pyi.
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.
Updated these comments.
| @overload | ||
| defwrite(self:_TemporaryFileWrapper[str],s:str)->int: ... | ||
| @overload | ||
| defwrite(self:_TemporaryFileWrapper[bytes],s:ReadableBuffer)->int: ... | ||
| @overload | ||
| defwritelines(self:_TemporaryFileWrapper[str],lines:Iterable[str])->None: ... | ||
| @overload | ||
| defwritelines(self:_TemporaryFileWrapper[bytes],lines:Iterable[ReadableBuffer])->None: ... |
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.
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.
Changed, and added some testcases forIO.write.
This comment has been minimized.
This comment has been minimized.
AlexWaygood commentedMar 9, 2023
Ping@JelleZijlstra, have you had a chance to take a look at my review feedback? :) |
JelleZijlstra commentedMar 9, 2023
Oops, I think I assumed this was still blocked. I will take a look soon! |
This comment has been minimized.
This comment has been minimized.
JelleZijlstra commentedMar 10, 2023
hm, the new way to write the overloads makes subclassing much worse. Let me see if I can fix that. |
This comment has been minimized.
This comment has been minimized.
Diff frommypy_primer, showing the effect of this PR on open source code: rich (https://github.com/Textualize/rich)+ rich/progress.py:173: error: Definition of "writelines" in base class "IOBase" is incompatible with definition in base class "IO" [misc]psycopg (https://github.com/psycopg/psycopg)+ psycopg/psycopg/copy.py:464: error: Unused "type: ignore" commentcwltool (https://github.com/common-workflow-language/cwltool)+ cwltool/main.py:963: error: Unused "type: ignore[arg-type]" comment |
JelleZijlstra commentedMar 10, 2023
Seems like we have a choice between:
I'm inclined to go with (1) because mypy's behavior is more clearly buggy than pyright's, but ideally we'd fix the mypy bug first. |
AlexWaygood commentedMar 12, 2023
Now thatpython/mypy#14882 has been merged, I guess we have two options:
I'm easy either way :) |
Fixes#9082