-
-
Notifications
You must be signed in to change notification settings - Fork 117
Add typing_extensions.utils #45
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
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! Some general feedback:
- I don't like the
ext
name. It's short for "extension", so it's extensions to the extensions? - I'm worried making typing-extensions into a package will break some downstream use of the library. I suppose we could just put the new names into typing_extensions.py, and then we don't have to think about the
ext
name either. - With these additions, we should also provide better documentation. Maybe a page on typing.readthedocs.io.
src/typing_extensions/ext.py
Outdated
... | ||
|
||
|
||
class SupportsRDivMod(Protocol[_T_contra, _T_co]): |
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.
This seems very obscure, and Python only uses it in two places. I think we can skip it.
In fact even SupportsDivMod
(without the R) seems marginal; it's also only used twice in typeshed.
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.
But it is used. I think it doesn't hurt to have them here instead of typing
. In fact, for completeness and consistency's sake I think it will make sense in the long run to have protocols for the other arithmetic operators here.
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 think SupportsRDivMod
is okay, though I agree with @JelleZijlstra that it isn't really used much, so I'd also be fine with omitting it.
A lot of the other arithmetic dunders are quite tricky, though (BinOp dunders in general are quite tricky), due to the presence of the reflected dunders. It can be hard to predict when you actually need an object with an __add__
method, and when "anything with either an __add__
or an __radd__
method" will do. SupportsRichComparison
in _typeshed
is a really good example: it has to be a union of protocols to work and, although it works really well for loads of functions in typeshed, we still have to use a different union of protocols for the functions in _operator.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.
This looks great! I agree with @JelleZijlstra's points, however. On the naming, maybe typing_extensions.utils
, on the grounds that these are small utilities that can be reused a bunch of times?
I'm also slightly nervous about including SupportsANext
, given that sometimes you want a guarantee that __anext__
will be a coroutine method, and sometimes you don't (python/typeshed#7491). It might not be as easy to use as users expect.
I like |
In cases, where |
I still think this would be somewhat surprising behaviour for users, but I'm okay with just adding a note to the documentation :) |
I also really like the idea of a subpackage, FWIW! |
So, any updates? Is the PR fine as it is or should I change things? In case it's considered a blocker, I can merge the subpackage, but I think that will cause more problems down the line, concerning maintainability/extensibility and distinction between typing backports and extensions. |
|
||
from os import PathLike | ||
from typing import AbstractSet, Awaitable, Iterable, Tuple, TypeVar, Union | ||
from typing_extensions import Protocol, TypeAlias |
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.
Maybe add __all__
?
... | ||
|
||
|
||
class SupportsDivMod(Protocol[_T_contra, _T_co]): |
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 still think we should drop the divmod protocols:
- They're not widely used: in typeshed we only use them for
divmod
itself. - They're unintuitive in that divmod() normally returns a pair, but the protocols don't reflect that.
@@ -144,4 +144,4 @@ These types are only guaranteed to work for static type checking. | |||
## Running tests | |||
|
|||
To run tests, navigate into the appropriate source directory and run | |||
`test_typing_extensions.py`. | |||
`test_typing_extensions/__init__.py`. |
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 can't comment there, but we should also mention the utils
module in the README around line 34.
See the comments of @gvanrossum in #47 regarding the naming of the subpackage. |
What's the status on this PR? It seems somewhat stale. |
At least from my perspective:
|
Not implying you should rush. Just couldn't find the reasoning for it being left in limbo in the comments here. Hoped for something explicit, like you just gave me. Appreciated. |
Closing this following the policy to only include things that are in CPython or in a PEP. Let's instead create a separate package for reusable types. |
Closes: #6