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

Overhaul some of the stubs for requests/urllib3 #2612

Closed
mgilson-argo opened this issue Nov 20, 2018 · 12 comments
Closed

Overhaul some of the stubs for requests/urllib3 #2612

mgilson-argo opened this issue Nov 20, 2018 · 12 comments
Labels
stubs: improvement Improve/refactor existing annotations, other stubs issues

Comments

@mgilson-argo
Copy link
Contributor

Specifically, it looks like the stubs for requests date back to a time when urllib3 was vendored with requests. These days, it looks like requests doesn't vendor it anymore and instead uses urllib3 from pypi.

I think that urllib3 should become it's own top-level stubs (not part of requests) and some of the requests stubs should probably be revisited. In order to limit scope, the particular stubs that I've been fighting with in requests.adapters -- BaseAdaptor.send doesn't accept proxies (even though it should), HTTPAdaptor.send has virtually no type annotations when BaseAdaptor.send does -- I think they should be basically the same...

What do you think?

@JelleZijlstra
Copy link
Member

The urllib3 issue is something we don't have a great answer for right now: what to do if different versions of a library have very different types? I'm not sure how long it has been since requests unvendored urllib3; if it's been a while I'd be more OK with removing the requests.packages.urllib3 stubs. In this case, people probably shouldn't have been using requests.packages.urllib3 anyway, since it was requests's private copy.

I haven't looked into the more specific issue you mentioned with adapters, but much of the requests stubs can probably be improved. PRs are welcome.

@mgilson-argo
Copy link
Contributor Author

Looking at the git history, it appears that urllib3 was split off/unvendored about 1 and a half years ago.

One of the problems with the current stubs are that the requests documentation ask you to pass things like a urrlib3.util.retry.Retry, but the stubs are still referencing the definitions from requests.packages which isn't correct these days. I would guess that the timescales we're talking about are long enough that it's probably safe to switch over (maybe asking stubgen to re-generate stubs for urllib3 or something rather than trying to port over the old stuff by hand)

As for

what to do if different versions of a library have very different types?

I think that's a very valid question -- And why I hope that people really adopt PEP-561. Then it's the library's problem rather than typeshed's ... Time will tell.

Is typing_extensions.Protocol fair game in typeshed? This doesn't actually solve the problem completely, but defining interfaces rather than concrete types could help with the vendored/unvendored problem (where you have types that are equivalent, but coming from disparate sources)

@JelleZijlstra
Copy link
Member

Yes, Protocols are fine to use in typeshed.

@ilevkivskyi
Copy link
Member

Moreover, you can just write from typing import Protocol in stubs.

@srittau srittau added size-large stubs: improvement Improve/refactor existing annotations, other stubs issues labels Nov 29, 2018
@koffie
Copy link

koffie commented May 21, 2021

Can this be closed since urllib3/urllib3#1897 is seeing more action than is seen here?

@Akuli
Copy link
Collaborator

Akuli commented May 21, 2021

If our requests stubs just import urllib3, should types-requests then depend on urllib3? I think types-foo packages can have dependencies only to other types-foo packages.

@ciaranevans
Copy link

Hey folks, I'm going around in circles in issues, this one is still open though 🤣

I'm getting error: Skipping analyzing "urllib3": found module but no type hints or library stubs when running mypy - Is there a working/in development package for urllib3 stubs?

@srittau
Copy link
Collaborator

srittau commented Sep 21, 2021

This depends on #5952. Then we can update the requests stubs to depend on urllib3 and remove the vendored urllib3 stubs.

@ciaranevans
Copy link

@srittau so something is in the works but not ready to go?

@srittau
Copy link
Collaborator

srittau commented Sep 21, 2021

In the works as in "I need to find the time to implement it".

@ciaranevans
Copy link

More than fair enough 😄 Just wanted to make sure I wasn't missing one ready to go!

@AlexWaygood
Copy link
Member

I believe this means that most of the problems discussed in this issue are now solved.

the particular stubs that I've been fighting with in requests.adapters -- BaseAdaptor.send doesn't accept proxies (even though it should), HTTPAdaptor.send has virtually no type annotations when BaseAdaptor.send does -- I think they should be basically the same...

I'm not sure if this is still a problem four years on. If it is, please feel free to open a new issue and/or PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stubs: improvement Improve/refactor existing annotations, other stubs issues
Projects
None yet
Development

No branches or pull requests

8 participants