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

Should HTTPMessage return Optional[str] for __getitem__? #2333

Closed
asottile opened this issue Jul 14, 2018 · 7 comments · Fixed by #11114
Closed

Should HTTPMessage return Optional[str] for __getitem__? #2333

asottile opened this issue Jul 14, 2018 · 7 comments · Fixed by #11114
Labels
stubs: false positive Type checkers report false errors

Comments

@asottile
Copy link
Contributor

>>> from urllib.request import urlopen
>>> urlopen('https://example.com')
<http.client.HTTPResponse object at 0x7fc608f39fd0>
>>> resp = urlopen('https://example.com')
>>> resp.headers['foo']
>>> resp.headers['connection']
'close'
>>> type(resp.headers['connection'])
<class 'str'>
import urllib.request
from typing import Optional

resp = urllib.request.urlopen('https://example.com')
x: Optional[str] = resp.headers['connection']
$ mypy t.py
t.py:5: error: Incompatible types in assignment (expression has type "Union[str, Header, None]", variable has type "Optional[str]")

If this sounds sane, I'd like to add __getitem__ here?

@JelleZijlstra
Copy link
Member

Does it never return Header? I think we discussed this in some issue before.

@asottile
Copy link
Contributor Author

I can't seem to find a situation where it returns Header, it's possible I'm overlooking something.

@OJFord
Copy link
Contributor

OJFord commented Aug 22, 2018

In a model generated from bytes, any header values that (in contravention of the RFCs) contain non-ASCII bytes will, when retrieved through this interface, be represented as Header objects with a charset of unknown-8bit.

from the last paragraph of intro, just above https://docs.python.org/3.5/library/email.message.html#email.message.Message.__len__

@asottile
Copy link
Contributor Author

Interesting! Though it doesn't appear to be true for HTTPResponse:

 while true; do echo -e "HTTP/1.1 200 OK\r\nContent-Length: 0\r\nWat: $(python -c 'print("\u2603")')\r\n" | nc -l localhost 1500; done
$ python3
Python 3.6.5 (default, Apr  1 2018, 05:46:30) 
[GCC 7.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from urllib.request import urlopen
>>> x = urlopen('http://localhost:1500')
>>> x.headers
<http.client.HTTPMessage object at 0x7f51af37ae80>
>>> x.headers['Wat']
'â\x98\x83'
>>> type(x.headers['Wat'])
<class 'str'>

Some pretty sick mojibake though :)

@srittau
Copy link
Collaborator

srittau commented Nov 19, 2018

I think the right solution is to override __getitem__() et al. in http.client.HTTPMessage to not return Header.

@srittau srittau added stubs: false positive Type checkers report false errors size-small labels Nov 19, 2018
@asottile
Copy link
Contributor Author

this seems worse now, it's Any so all type checking is gone -- this appears to be due to #3045

@Avasam
Copy link
Collaborator

Avasam commented Dec 1, 2023

Taking a look at this test run: https://github.com/Avasam/typeshed/pull/36/files#diff-ced02b01b42b244e9b7bc2e12ef7a215bde89a29072c99ade903f58fb83a3bc7R36

spack would have to change their assert to first store the value in a local variable then assert that variable: https://github.com/spack/spack/blob/develop/lib/spack/spack/oci/oci.py#L138-L142

twine implicitely uses header_factory. https://github.com/pypa/twine/blob/main/twine/commands/check.py#L73C40-L73C54 They could replace with

    _, header = msg.policy.header_store_parse("content-type", value)`
    return msg.get_content_type(), header.params

urllib3 is a false-positive, and an interesting one: https://github.com/urllib3/urllib3/blob/main/src/urllib3/contrib/emscripten/fetch.py#L382C24-L382C24


I'm tempted to say that, from a user code PoV, you should just use .get instead of accessing through [] to be type-safe. And that usual expectation with ["key"] is that you're assuming that the value is present otherwise it throws (except that it returns None here instead of throwing).


As for the type currently being Any. I don't think it could really be resolved without something like AnyOf, or making Message generic with a default value (wasn't there a PEP for that?)

If HTTPMessage really is always strings, we could override all Message methods that use _HeaderType

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stubs: false positive Type checkers report false errors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants