TYPING: add some type hints to core.generic
State: closed

pre-cursor to #27527

Labels:
name: Typing
description: type annotations, mypy/pyright type checking

Comments:
I think typing_extensions has some good stuff (the `Literal` application here is great, have seen good use cases for `Final` in the past) but noting that we did try this before in #25975 and I think there was concern about adding another dependency

cc @TomAugspurger to see if that's still a concern
From what I recall, the benefits of adding that extension were relatively
minor. I haven't looked at the changes here to judge whether they're worth
it.

But in general, I would say that taking on a new dependency should have a
high hurdle. For what's essentially a dev-only dependency, we could vendor
a version of that package.

On Mon, Aug 5, 2019 at 10:49 AM William Ayd <notifications@github.com>
wrote:

> I think typing_extensions has some good stuff (the Literal application
> here is great, have seen good use cases for Final in the past) but noting
> that we did try this before in #25975
> <https://github.com/pandas-dev/pandas/pull/25975> and I think there was
> concern about adding another dependency
>
> cc @TomAugspurger <https://github.com/TomAugspurger> to see if that's
> still a concern
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/27646?email_source=notifications&email_token=AAKAOIT32ZPAUZPNUHRIROLQDBDZTA5CNFSM4IHVTH52YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3SHXGA#issuecomment-518290328>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAKAOIQLKEQF3QIY62KT2BLQDBDZTANCNFSM4IHVTH5Q>
> .
>

It does look likes Literal is still considered an alpha feature:

https://mypy.readthedocs.io/en/latest/literal_types.html

So no strong preference on my end but maybe safer to forgo that for now and not take on the extra dep. Can make more strict as that stabilizes in the future and maybe makes its way into stdlib
> For what's essentially a dev-only dependency,

it's only added to the dev environment. not to any of the other builds.
A user would need it though, right? Else the name wouldn't be defined?
> have seen good use cases for `Final` in the past

since we only do the mypy check on py3.7, we can add `Final` etc. anyway... and get around the runtime behaviour on py3.5 and py3.6 in the same way as getting around typing_extensions here.

the methodology used will be required under some scenarios anyway.. https://mypy.readthedocs.io/en/latest/common_issues.html#using-classes-that-are-generic-in-stubs-but-not-at-runtime



> A user would need it though, right? Else the name wouldn't be defined?

only if performing type checking.
> It does look likes Literal is still considered an alpha feature:

but I think should have utility in overloads to reduce the number of casts or ignores needed.
Thinking it over some more and knowing this is a dev-only dependency I don't think adding this is worth it, especially since any time we would use we would have to wrap in a TYPE_CHECKING check and provide an alias when not type checking. I think this is less readable and perhaps confusing to those less versed in annotations

I think the rest looks good without typing_extensions
> Thinking it over some more and knowing this is a dev-only dependency I don't think adding this is worth it, especially since any time we would use we would have to wrap in a TYPE_CHECKING check and provide an alias when not type checking. I think this is less readable and perhaps confusing to those less versed in annotations

I'll revert for now but we will need a better reason not to include Literal IMHO.

see https://github.com/python/typing/issues/478#issuecomment-339817759 and https://docs.google.com/document/d/1vpMse4c6DrWH5rq2tQSx3qwP_m_0lyn-Ij4WHqQqRHY/mobilebasic

> From what I recall, the benefits of adding that extension were relatively minor.

from PEP 586 -- Literal Types, Motivation and Rationale https://www.python.org/dev/peps/pep-0586

> This pattern is also fairly common in many popular 3rd party libraries. For example, here are just two examples from pandas and numpy respectively:

> pandas.concat(...) will return either Series or DataFrame depending on whether the axis argument is set to 0 or 1.

> numpy.unique will return either a single array or a tuple containing anywhere from two to four arrays depending on three boolean flag values.


@shoyer put the case for pandas. so I think we should use it.
> I think typing_extensions has some good stuff (the `Literal` application here is great, have seen good use cases for `Final` in the past) but noting that we did try this before in #25975 and I think there was concern about adding another dependency
> 
> cc @TomAugspurger to see if that's still a concern

typing_extensions will be availble for use with mypy 0.720 without needing to explicity add as a dependancy

https://github.com/python/mypy/blob/36aecdae92d050f626ea9029ad08f0c8382c47e7/setup.py#L181-L184
Would still cause import errors if someone doesn’t have Mypy installed then though right?

Sent from my iPhone

> On Sep 12, 2019, at 12:27 PM, Simon Hawkins <notifications@github.com> wrote:
> 
> I think typing_extensions has some good stuff (the Literal application here is great, have seen good use cases for Final in the past) but noting that we did try this before in #25975 and I think there was concern about adding another dependency
> 
> cc @TomAugspurger to see if that's still a concern
> 
> typing_extensions will be availble for use with mypy 0.720 without needing to explicity add as a dependancy
> 
> https://github.com/python/mypy/blob/36aecdae92d050f626ea9029ad08f0c8382c47e7/setup.py#L181-L184
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.

> Would still cause import errors if someone doesn’t have Mypy installed then though right?

would assume that would still need to use if TYPE_CHECKING, yes.