-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
make io classes inherit from typing IO classes #4145
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
Conversation
This makes these classes usable if type annotations are given as "IO" or "TextIO". In the future, we'll then be able to move open() to return a concrete class instead (python#3951).
Fixes python#3951. We use the values of the "mode" and "buffering" arguments to figure out the concrete type open() will return at runtime. (Compare the CPython code in https://github.com/python/cpython/blob/master/Modules/_io/_iomodule.c#L231.) I tested by writing a script that generated every mode/buffering combination, then running mypy with the open plugin disabled and comparing the results. This PR depends on python#4145.
Fixes python#3951. We use the values of the "mode" and "buffering" arguments to figure out the concrete type open() will return at runtime. (Compare the CPython code in https://github.com/python/cpython/blob/master/Modules/_io/_iomodule.c#L231.) I tested by writing a script that generated every mode/buffering combination, then running mypy with the open plugin disabled and comparing the results. This PR depends on python#4145.
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, just one question below. Feel free to commit either way.
# Technically this is whatever is passed in as file, either a str, a bytes, or an int. | ||
name: Union[int, str] # type: ignore |
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.
Why not type it as Union[str, bytes, int]
then?
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.
I didn't want to broaden the type because nobody has complained about it, it's not directly related to the goal of this PR, and there's some chance broadening the type could cause new errors for people using the attribute.
Technically the right solution is to make various IO classes (not just this one but also at least TextIOWrapper) generic over (str, bytes, int) and give .name
that generic type. But making these classes generic has the cost that now all users with --disallow-any-generics
on have to specify the type parameter. That doesn't seem worth it for a rarely used attribute that nobody has even opened an issue about.
This (3058bec) causes an error with the following now: import io
class MyStringIO(io.StringIO):
def readline(self, size=None) -> str:
return super().readline(size)
(with mypy master (0.790+dev.3adb2e952cf)) |
That's strange, because I tried adding |
This makes these classes usable if type annotations are given as "IO" or "TextIO". In the future, we'll then be able to move open() to return a concrete class instead (python#3951).
@JelleZijlstra given that this has been released with mypy 0.790 now, should there be an issue filed for it? (assuming it is yet not clear what the problem is / it cannot be fixed simply) |
Yes, sounds good. |
ok, filed python/mypy#9643. |
This makes these classes usable if type annotations are given as "IO"
or "TextIO". In the future, we'll then be able to move open() to
return a concrete class instead (#3951).
Also removed a number of redundant methods.