Skip to content
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

ENH: Vendor typing_extensions #34869

Closed
WillAyd opened this issue Jun 19, 2020 · 8 comments · Fixed by #36000
Closed

ENH: Vendor typing_extensions #34869

WillAyd opened this issue Jun 19, 2020 · 8 comments · Fixed by #36000
Labels
Enhancement Needs Discussion Requires discussion from core team before further action
Milestone

Comments

@WillAyd
Copy link
Member

WillAyd commented Jun 19, 2020

I’ve discussed this with @simonjayhawkins here and there but never had a formal issue opened, hence this :-)

I think we need to vendor typing_extensions to realistically be able to unpin mypy from where it is now. The main thing I think we need is to unlock Protocols; these aren’t added until Python 3.8 but as of mypy 0.75 seem to be the documented solution to a lot of the Mixin problems that we face:

https://mypy-lang.blogspot.com/2019/11/mypy-0.html

There are some other useful things like Literal, Final and TypedDict that this would unlock as well, to at least hold us over until 3.8 is the minimum supported version

@WillAyd WillAyd added Enhancement Needs Triage Issue that has not been reviewed by a pandas team member Needs Discussion Requires discussion from core team before further action and removed Needs Triage Issue that has not been reviewed by a pandas team member labels Jun 19, 2020
@jbrockmendel
Copy link
Member

is there an important distinction between vendoring vs making it a dependency?

@WillAyd
Copy link
Member Author

WillAyd commented Jun 19, 2020 via email

@simonjayhawkins
Copy link
Member

is there an important distinction between vendoring vs making it a dependency?

cc @TomAugspurger for added input here.

from #27646 (comment).

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.

For me, the conversation that followed that comment is my understanding of the basis of our current (collective) position.

It is good to revisit this discussion, as with time our positions/views may have changed.

I think that now, I'm leaning to avoiding the dependency for now, as there are many other typing tasks that can still be progressed and typing_extensions need not be considered a blocker. But vendoring it is also fine with me.

The main thing I think we need is to unlock Protocols; these aren’t added until Python 3.8 but as of mypy 0.75 seem to be the documented solution to a lot of the Mixin problems that we face:

i'm not keen to 'solve' our mixin issues using Protocols until the issue has had sufficient discussion in isolation. (and, for me, should not be adopted before we have a clear picture on the use of Generic which is a "fundamental building block" of the typing module https://www.python.org/dev/peps/pep-0484/#the-typing-module)

Protocol obviously do have benefits. but for me, they are for extending the available built-in generics, with 'simple' protocols in the first instance.

For complex typing issues, I think the google approach makes more sense. see Power Features http://google.github.io/styleguide/pyguide.html#219-power-features, but I think that for pandas, being a contributor friendly project, applies to all dynamic code.

For this typing could help, see http://google.github.io/styleguide/pyguide.html#2212-pros

2.21.2 Pros
Type annotations improve the readability and maintainability of your code. The type checker will convert many runtime errors to build-time errors, and reduce your ability to use Power Features.

So maybe instead of 'solving' typing issues, a bigger discussion on code style/structure is needed.

Interestingly, a quick glance at the Modin codebase is easier to grok/navigate in places with less dynamic code e.g.

https://github.com/modin-project/modin/blob/7f735fcbc5ccbcdf525d2e769f1745b74698ffb0/modin/pandas/dataframe.py#L2389-L2398

and

https://github.com/modin-project/modin/blob/7f735fcbc5ccbcdf525d2e769f1745b74698ffb0/modin/pandas/dataframe.py#L489

@TomAugspurger
Copy link
Contributor

I would be fine with vendoring if it's valuable for typing. I don't think it's appropriate to add it as a required dependency.

@jbrockmendel
Copy link
Member

@simonjayhawkins thanks for explaining. im fine with vendoring.

@jbrockmendel
Copy link
Member

@WillAyd anything stopping you/us from moving forward with this?

@WillAyd
Copy link
Member Author

WillAyd commented Aug 5, 2020 via email

@simonjayhawkins
Copy link
Member

simonjayhawkins commented Aug 5, 2020

pytest has recently addded types and is using Protocol without the runtime dependency or vendoring

https://github.com/pytest-dev/pytest/blob/303030c14130a5777bdaace678b9f4adb07416ab/src/_pytest/outcomes.py#L12-L22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Needs Discussion Requires discussion from core team before further action
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants