Improve typing of run by using @overload#685
Improve typing of run by using @overload#685gabrieldemarmiesse merged 2 commits intogabrieldemarmiesse:masterfrom
run by using @overload#685Conversation
patrick91
force-pushed
the
feature/overlods
branch
from
40a444e to
4d11d93
Compare
|
|
||
| import pydantic | ||
| from typing_extensions import TypeAlias | ||
| from typing_extensions import TypeAlias, Unpack |
There was a problem hiding this comment.
In the original PR this was conditionally imported from typing if the Python version was new enough, but typing_extension does the same already 😊
There was a problem hiding this comment.
The idea of the version check was to be able to remove typing-extensions as a dependency in versions of Python that don't need it (e.g. https://github.com/aio-libs/multidict/blob/18344e20b7a0c883b48de685174a78b8b33663e7/setup.cfg#L45), plus allow tools like pyupgrade/ruff to automatically remove the old imports when the project stops supporting a particular version of Python.
but typing_extension does the same already
Obviously typing-extensions will have all of these things already, given that's it's sole purpose it to backport typing features to older versions of Python.
There was a problem hiding this comment.
@Dreamsorcerer maybe we can follow up with another PR? since there's already typing_extensions installed?
There was a problem hiding this comment.
Yes, it's not up to me, just providing some feedback that could be adopted by the project.
There was a problem hiding this comment.
Yeah I'd been going with the approach of being explicit about the versions where typing_extensions is actually needed, but I realise it does add clutter and overhead. I can see the argument both ways, but when in doubt going with project consistency is generally a safe bet.
There was a problem hiding this comment.
I'm not sure anyone is complaining about having typing_extensions as a dependency for recent python versions. If someone opens an issue and express a use case where this is annoying we'll take a look at it. Let's not fix a problem that's not bothering anyone. Importing typing_extensions unconditionally is fine for now.
| **kwargs: Unpack[RunArgs], | ||
| ) -> str: ... | ||
|
|
||
| def run( |
There was a problem hiding this comment.
Is there a reason the non-overload method doesn't use **kwargs: Unpack[RunArgs]? If it doesn't there's still duplication and a need to update two places at once.
There was a problem hiding this comment.
@LewisGaul yes, unfortunately it is needed for docs, I didn't explain it properly in my PR message, but basically since it's not possible to give default args to kwargs we can't really avoid the duplication
|
Thanks a lot for the PR! Sorry for the delayed review, I'm currently on holiday without access to my computer. I'll take a look at the beginning of next week |
gabrieldemarmiesse
left a comment
gabrieldemarmiesse
left a comment
There was a problem hiding this comment.
The duplication is acceptable, we don't degrade the UX in any way, docs or auto-completion. So yeah let's merge this and hope futur changes to the python language allow us to avoid this duplication. Thanks @Dreamsorcerer @patrick91 for your work on this!
gabrieldemarmiesse
merged commit b694f85
into
gabrieldemarmiesse:master
|
I'll wait for a few days, if you want to try it out and then do a release end of this week. Cheers! |
|
@gabrieldemarmiesse thanks, just tested it and it works as expected! |
4 participants
Closes #580
Credits go to @Dreamsorcerer
I didn't think of this first, but mixing the
typing.Unpackapproach with the default signature works around the missing default values in the docs:From what I understood there's no way of using
typing.Unpackand passing default values, but in this case we still get some benefits from using them since we'd need to duplicate a lot of args (I might do something similar in one of my projects :D )