-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Replace various Incompletes in stdlib #11673
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
def open( | ||
self, | ||
mode: OpenTextMode = "r", | ||
*, | ||
encoding: str | None = ..., | ||
errors: str | None = ..., | ||
newline: str | None = ..., | ||
line_buffering: bool = ..., | ||
write_through: bool = ..., | ||
) -> TextIOWrapper: ... | ||
@overload | ||
def open(self, mode: OpenBinaryMode, *args: Unused, **kwargs: Unused) -> BinaryIO: ... | ||
def open( | ||
self, | ||
mode: OpenTextMode, | ||
buffer: _WrappedBuffer, | ||
encoding: str | None = ..., | ||
errors: str | None = ..., | ||
newline: str | None = ..., | ||
line_buffering: bool = ..., | ||
write_through: bool = ..., | ||
) -> TextIOWrapper: ... |
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.
For text modes, the arguments are just passed on to TextIOWrapper.__init__()
. That said, I'm not sure if passing a buffer here is really a good idea. But then, the implementation of this method is strange anyway, creating a stream when using the text mode, but then not passing that stream to TextIOWrapper
...
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.
It was indeed a CPython bug: python/cpython#111775.
@overload | ||
def open(self, mode: str, *args: Incomplete, **kwargs: Incomplete) -> IO[Any]: ... | ||
def open(self, mode: OpenBinaryMode) -> BinaryIO: ... |
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.
While args and kwargs are just ignored in this case, passing any further arguments is potentially a bug, which is why I removed the arguments and their Unused
annotations.
def __typing_subst__(self, arg: Any) -> Any: ... | ||
def __typing_prepare_subst__(self, alias: Any, args: Any) -> tuple[Any, ...]: ... |
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.
These dunders are unfortunately completely undocumented, but it seems that only type forms etc. are passed through, so Any
is probably the most appropriate annotation for the time being.
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.
Yeah, these are very much implementation details that users should steer clear of... Possibly we could/should just remove them from the stubs and allowlist them. But I'm also fine with just using Any
.
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
def open( | ||
self, | ||
mode: Literal["r"] = "r", | ||
encoding: str | None = None, | ||
errors: str | None = None, | ||
newline: str | None = None, | ||
line_buffering: bool = False, | ||
write_through: bool = False, | ||
) -> TextIOWrapper: ... | ||
@overload | ||
def open(self, mode: OpenBinaryMode, *args: Unused, **kwargs: Unused) -> BinaryIO: ... | ||
def open(self, mode: Literal["rb"]) -> BinaryIO: ... |
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.
These overloads are a lot more selective than they used to be. On main
, the first overload matches if any of these strings is passed in as a literal string:
typeshed/stdlib/_typeshed/__init__.pyi
Lines 164 to 199 in 0cea0bc
OpenTextModeUpdating: TypeAlias = Literal[ | |
"r+", | |
"+r", | |
"rt+", | |
"r+t", | |
"+rt", | |
"tr+", | |
"t+r", | |
"+tr", | |
"w+", | |
"+w", | |
"wt+", | |
"w+t", | |
"+wt", | |
"tw+", | |
"t+w", | |
"+tw", | |
"a+", | |
"+a", | |
"at+", | |
"a+t", | |
"+at", | |
"ta+", | |
"t+a", | |
"+ta", | |
"x+", | |
"+x", | |
"xt+", | |
"x+t", | |
"+xt", | |
"tx+", | |
"t+x", | |
"+tx", | |
] | |
OpenTextModeWriting: TypeAlias = Literal["w", "wt", "tw", "a", "at", "ta", "x", "xt", "tx"] | |
OpenTextModeReading: TypeAlias = Literal["r", "rt", "tr", "U", "rU", "Ur", "rtU", "rUt", "Urt", "trU", "tUr", "Utr"] |
And similarly, the second overload on main
matches if any of these strings is passed in as a literal string:
typeshed/stdlib/_typeshed/__init__.pyi
Lines 201 to 228 in 0cea0bc
OpenBinaryModeUpdating: TypeAlias = Literal[ | |
"rb+", | |
"r+b", | |
"+rb", | |
"br+", | |
"b+r", | |
"+br", | |
"wb+", | |
"w+b", | |
"+wb", | |
"bw+", | |
"b+w", | |
"+bw", | |
"ab+", | |
"a+b", | |
"+ab", | |
"ba+", | |
"b+a", | |
"+ba", | |
"xb+", | |
"x+b", | |
"+xb", | |
"bx+", | |
"b+x", | |
"+bx", | |
] | |
OpenBinaryModeWriting: TypeAlias = Literal["wb", "bw", "ab", "ba", "xb", "bx"] | |
OpenBinaryModeReading: TypeAlias = Literal["rb", "br", "rbU", "rUb", "Urb", "brU", "bUr", "Ubr"] |
What's the reason for changing it so that the first overload only matches if a literal "r"
is passed in, and so that the second overload only matches if a literal "rb"
is passed in?
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.
It's now matching the documentation of Traversable
. All other modes make no sense and are likely a bug: https://github.com/python/cpython/blob/dd44ab994b7262f0704d64996e0a1bc37b233407/Lib/importlib/resources/simple.py#L88-L92
(You could argue for allowing br
, but there's really no case for using this non-standard ordering.)
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.
In theory, we could also allow ""
and "b"
, but this is also non-standard according to the docs.
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.
Okiedokie
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 good other than my two questions above
Co-authored-by: Alex Waygood <[email protected]>
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!
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
No description provided.