Improve typing of run()

[Dreamsorcerer]

No description provided.

[gabrieldemarmiesse]

Hi, thanks for this PR, I'm not opposed to such a change, but I'm a big worried the duplication will cause issues. (we'll update a signature and forget to update the other). I'm happy to merge such a change when we drop support to python 3.10 (~1.5 years for now). Sorry for that, we need to keep the project as low-maintenance as possible

[Dreamsorcerer]

I'm not opposed to such a change, but I'm a big worried the duplication will cause issues.

As I mention in the description, if you don't mind a partial loss of typing information on Python <3.11, then we can remove that duplication today. So Python 3.11+ gets the full typing support with correct return types, while older version of Python will see def run(**kwargs: Any) -> Union[...]. This loses some precision for older users (who already have awkwardly unusable return types), but increases precision for newer users.

[gabrieldemarmiesse]

I understand, but that would be considered a regression. And python 3.10 is still alive for two more years, so those users are still first class citizens in the eyes of the project. We'll stay with the status quo for a bit longer

[Dreamsorcerer]

No problem, I'll leave it up to you. But, from my perspective, the loss on 3.10 is minor (essentially .run(this_is_not_valid=1) won't produce a type error anymore) whereas the current problem with return types is unavoidable (the types in the union are completely incompatible, so doing anything with the return value will cause a type error incorrectly unless asserting the correct type first (see, for example, errors here: https://github.com/aio-libs/aiohttp/actions/runs/8766737649/job/24059207415?pr=8365)).

[Taragolis]

There is no problem to use Unpack in versions below Python 3.11, almost everything from stdlib typing could be imported from typing_extensions module

[Dreamsorcerer]

That is also an option, yes. From looking around the project, it seemed like it was avoiding adding a new dependency on typing_extensions. @gabrieldemarmiesse Would you be fine with adding typing-extensions for Python <3.12?

[gabrieldemarmiesse]

We already have typing-extensions as a dependency, feel free to use it :)

[Dreamsorcerer]

Ah, maybe I misread it earlier. Will update later.

[Dreamsorcerer]

That should be fine now. I'd note that it's probably worth introducing mypy to the project. I tried to check the code with mypy and there appeared to be >200 type errors already present in the code (without adding an extra strictness settings).

[gabrieldemarmiesse]

Thank you, before merging this, I'll try to see how it behaves in different IDEs. You don't need to do anything, I'm just running some tests to see if it's well supported :)

[gabrieldemarmiesse]

Sorry for the huge delay. I tried your PR with Pycharm and here is what I get:

As you can see, pycharm sees **kwargs but doesn't understand the TypedDict behind it. I'm using python 3.10.13 here, and pycharm 2024.1.1.

So I'm all for improving the typing experience, so in my eyes this PR still has a lot of value, but we'll wait until at vscode and pycharm both can understand the function signature before pushing forward.

I think you can convert this pull request to "Draft" and we'll reopen it when the two majors IDEs support this feature. Does that sound good to you?

[Dreamsorcerer]

So I'm all for improving the typing experience, so in my eyes this PR still has a lot of value, but we'll wait until at vscode and pycharm both can understand the function signature before pushing forward.

I feel like we have different priorities. As someone that is checking code with mypy and trying to have type safe code, the current state is unusable, as you will always get type errors due to the return type being incorrect. Whereas seeing the kwargs in an IDE seems like a nice-to-have, you can always look at the documentation if it's missing.

But, a quick search suggests that Pycharm 2024.1.2 already has support: https://blog.jetbrains.com/pycharm/2024/05/pycharm-2024-1-2/#code-assistance-for-typeddict-and-unpack

I'm pretty sure VSCode is built on top of pyright, which has supported Unpack for probably atleast a year, so I'd also be surprised if that really doesn't work with it (maybe it will error when you put the wrong kwarg in, even if it doesn't display the kwargs?).

[Dreamsorcerer]

pycharm 2024.1.1

So, looks like you were one patch release short of the Unpack support.

[gabrieldemarmiesse]

I'll retry it then, thanks for the info!

…

On Sun, 21 Jul 2024, 13:51 Sam Bull, ***@***.***> wrote: pycharm 2024.1.1 So, looks like you were one patch release short of the Unpack support. — Reply to this email directly, view it on GitHub <#580 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADCLMK4CU67IPBM34MGZMGLZNOOETAVCNFSM6AAAAABGQSZY7OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBRGU4DCNJVGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[gabrieldemarmiesse]

@Dreamsorcerer I retried and it works on VSCode and Pycharm :) I think we can merge this PR. Can you rebase/merge master into your branch? Afterwards we should be good to go :)

[gabrieldemarmiesse]

I forgot to check if the documentation generation works well with it. I'm sorry about that, the PR has far reaching consequences that are sometimes hard to find.

[gabrieldemarmiesse]

It seems we lose all the default values for the arguments in the docs. It looks like this now:

I wonder if it's better to duplicate all the default values in the docstring, or to just say that we won't be mypy-compatible until Python creates a typing feature that allows us to dedupe arguments in overloads. I'll think about it.

[Dreamsorcerer]

Yes, that'd be expected. I'm not aware of any plans currently to introduce defaults in this way, so I wouldn't want to wait on that.

[Dreamsorcerer]

The vast majority of them default to None or an empty list, which is probably not so important to document. I'd consider just manually documenting defaults for the few that are not obvious.

[LewisGaul]

FWIW I agree with @gabrieldemarmiesse in being hesitant about this change... It's a slight sacrifice of readability in the function signature for the sake of cutting-edge and slightly immature typing features, not a totally obvious choice to me. I understand the issue with using a type checker under status quo, but I just use typing.cast().

[Dreamsorcerer]

but I just use typing.cast().

Sure, but then you have no type safety. A safer approach would be litter the code with isinstance() asserts, but it's rather annoying and unnecessary when the correct information is available.

[LewisGaul]

FWIW I agree with @gabrieldemarmiesse in being hesitant about this change... It's a slight sacrifice of readability in the function signature for the sake of cutting-edge and slightly immature typing features, not a totally obvious choice to me. I understand the issue with using a type checker under status quo, but I just use typing.cast().

Having been using this package in a more tightly type-checked codebase I'd like to take back my hesitation and make myself +1 for this kind of change. Python's typing is unquestionably problematic in some areas, but in this case I agree with @Dreamsorcerer that this provides net benefit.

The vast majority of them default to None or an empty list, which is probably not so important to document. I'd consider just manually documenting defaults for the few that are not obvious.

I'm on board with this for this case where defaults are generally unimportant, simply representing "not passed". (In fact, the distinction between "not passed" and "default value" should be made clearer with all args defaulting to None, see #620 where passing an empty list should have had meaning).

Could we add overloads for Container.execute() as well?