-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
refactor: use typing_extensions.Self
#9251
base: main
Are you sure you want to change the base?
Conversation
b85f4d1
to
fe9f85b
Compare
typing_extensions.Self
typing_extensions.Self
citation required
if poetry imports from typing-extensions then it should express that dependency directly. |
Sure:
Yeah, I had been aware of that and happened to mention it, but I see it as irrelevant and personally think it shouldn't be relied on. AFAIK, it's not the direct reason
First things first, if we rely on pyright additionally to mypy, I believe it should be included in the CI suite. Probably some issues that pyright would typically report (and mypy wouldn't due to some of their discrepancies) could have been (or can in the future be) merged undetected.
Well, that's weird since the |
no part of what you quote says "therefore you do not need Most of the rest of what you write confuses stubs for typing-extensions with the typing-extensions package, I think it is - for the moment, just about - true that it would be sufficient to put
why even worry about adding the dependency? |
It's a conclusion. It just isn't explicitly stated in the docs.
Sorry, but I'm afraid you've completely missed my point.
When
Because it is unnecessary. Poetry already relies on a lot of packages. If you're still hesitant that installing I hope that that is convincing enough, if I myself hadn't been. |
as we have established, actual typecheckers do not approve of this approach: As I say, declaring the dependency only in the typing dependency group would I think be sufficient, though I still do not like it (for the reasons I already gave). CI might or might not catch missing dependencies. Eg it is very plausible to fail to spot a missing dependency on a package that is made transitively available by a test dependency. |
I don't think that warning would impact users of poetry? But yeah, it's true that that's a lint that pyright has and mypy doesn't
FWIW, while @bswck is correct that it is strictly speaking unnecessary to list Anyway, I think both ways of doing it are workable, and it's up to you folks to weigh the tradeoff appropriately :-) ping me again if you need me! |
pyright lint seems to have been in response to a bug report saying "you are failing to detect that typing-extensions is missing" - microsoft/pyright#7318 possibly that fix was an overshoot and they could be convinced that the lint is only desirable for imports made outside of |
Used
typing_extensions.Self
instead of(self: T, ...) -> T
.typing_extensions
has a special stub in typeshed and is seen by mypy even if not installed as a direct/transitive dependency, so there's no need to change anything in dependency specs.typing_extensions
was used instead oftyping
because theSelf
type was introduced in 3.11 with PEP 673 andtyping_extensions
offers a backport for 3.8+, which is our case.Operation.skip()
andOperation.unskip()
have an anti-pattern, i.e. they returnSelf
despite operating in-place.This will be addressed in a separate PR; I need an idea how to address the API incompatibility that would bring.