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

Urllib2 #2688

Merged
merged 9 commits into from Feb 18, 2019

Conversation

@pmhahn
Copy link
Contributor

commented Dec 12, 2018

Several more type annotation for urllib2 in Python2.
The first 4 patches should be uncontroversial.
Modeling the Response helped me find some bug where I accessed internal data.
Please have a close look at the last patch as Message is not a subclass of dict, so it might breaks old uses where using dict was okay. urllib2 itself will always pass a Message instance, but custom code might create its own dictionary and pass it back to further calls.

@srittau
Copy link
Collaborator

left a comment

Thank you for the pull request!

For the headers/Message problem: I believe the best solution is to use Mapping[str, str] for parameter types and fields. (MutableMapping[str, str] could make some sense for fields, too.) Message should fulfil that protocol as does Dict[str, str]. (I have not marked all places where this occurs below.)

stdlib/2/urllib2.pyi Outdated Show resolved Hide resolved
stdlib/2/urllib2.pyi Outdated Show resolved Hide resolved
stdlib/2/urllib2.pyi Outdated Show resolved Hide resolved
stdlib/2/urllib2.pyi Outdated Show resolved Hide resolved

pmhahn added a commit to univention/typeshed that referenced this pull request Dec 18, 2018

stdlib/2/urllib2.pyi Outdated Show resolved Hide resolved
stdlib/2/urllib2.pyi Outdated Show resolved Hide resolved
stdlib/2/urllib2.pyi Outdated Show resolved Hide resolved
@srittau
Copy link
Collaborator

left a comment

Thank you for your persistence with this patch. LGTM, just three more (optional) comments regarding _string.

stdlib/2/urllib2.pyi Outdated Show resolved Hide resolved
stdlib/2/urllib2.pyi Outdated Show resolved Hide resolved
stdlib/2/urllib2.pyi Outdated Show resolved Hide resolved
stdlib/2/urllib2.pyi Outdated Show resolved Hide resolved

pmhahn added some commits Dec 18, 2018

urllib2: Fix OpenerDirector.open(fullurl) name
The parameter is called "fullurl".
urllib2: CacheFTPHandler.setMaxConns(int)
The maximum number of connections must be a non-negative integer.
urllib2: add OpenerDirector.addheaders
<https://docs.python.org/2/library/urllib2.html#example> contains an
example using 'addheaders', so it is part of the public API.
urllib2: add Abstract*AuthHandler.add_password and realm=None
<https://docs.python.org/2/library/urllib2.html#example> contains an
example using 'HTTPBasicAuthHandler.add_password', so it is part of the
public API.

It references the method HTTPPasswordMgr.add_password() directly:
> 870: self.add_password = self.passwd.add_password

Also realm=None is allowed, which is required when
HTTPPasswordMgrWithDefaultRealm is used:
<https://docs.python.org/2/howto/urllib2.html#id6>:
> Frequently one doesn’t care what the realm is.
...
> We indicate this by providing None as the realm argument to the
> add_password method.
urllib2: ProxyHandler.proxies is a Mapping[str, str]
as returned by urllib.getproxies():
> def getproxies() -> Mapping[str, str]: ...

The constructor has even contains this code:
> assert hasattr(proxies, 'has_key'), "proxies must be a mapping"

But "has_key()" is not enough as then "proxies.items()" is used.

The reference of that mapping is stored as the instance variable
'proxies'.
urllib2: req[uest] is of type Request
The maximum number of connections must be a non-negative integer.
urllib2: Use addbase to model readable IO
<https://docs.python.org/2/library/urllib2.html#urllib2.urlopen>
> urllib2.urlopen(url[,data[,timeout[,cafile[,capath[,cadefault[,context]]]]])
...
> This function returns a file-like object with three additional
> methods:
> - geturl()
> - info()
> - getcode()
...
> Note that None may be returned if no handler handles the request.

First suggested by srittau at
<#2688 (review)>
but then changed to just use `class addbase` inspired by commit
1442cc0 from Michael Brandt.
urllib2: handle headers as Mapping
Internally mimetools.Message is used, which is a dict-like object, but
> All header matching is done independent of upper or lower case

<https://docs.python.org/2/library/urllib2.html#urllib2.BaseHandler.http_error_default>
> fp will be a file-like object with the HTTP error body
> code will be the three-digit code of the error
> msg will be the user-visible explanation of the code
> hdrs will be a mapping object with the headers of the error

@pmhahn pmhahn force-pushed the univention:urllib2 branch from f6a29d0 to 2b4fcd6 Feb 18, 2019

@pmhahn

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2019

I rebased the whole branch due to one more merge conflict with master. I squashed all incremental commits and changed the code to use addinfourl instead of my own protocol _SupportsRead as inspired by 1442cc0.

@srittau
Copy link
Collaborator

left a comment

I gave it another quick glance, and nothing wrong stood out. Going to merge now.

@srittau srittau merged commit 25c9640 into python:master Feb 18, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pmhahn pmhahn deleted the univention:urllib2 branch Feb 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.