FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Make `http.cookies.SimpleCookie` non-generic by flaeppe · Pull Request #10701 · python/typeshed · GitHub

Repository navigation

Make http.cookies.SimpleCookie non-generic - #10701

Merged
srittau merged 1 commit into
python:mainfrom
flaeppe:fix/simple-cookie-not-generic
Sep 13, 2023
Merged

srittau merged 1 commit into
python:mainfrom
flaeppe:fix/simple-cookie-not-generic

Conversation

flaeppe commented Sep 11, 2023

Copy link
Copy Markdown

Fixes: #10700

Copy link
Copy Markdown
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

steam.py (https://github.com/Gobot1234/steam.py)
+ steam/http.py:180: error: The type "type[SimpleCookie]" is not generic and not indexable  [misc]
+ steam/http.py:181: error: The type "type[SimpleCookie]" is not generic and not indexable  [misc]

aiohttp (https://github.com/aio-libs/aiohttp)
+ aiohttp/helpers.py:942:24: error: "SimpleCookie" expects no type arguments, but 1 given  [type-arg]
+ aiohttp/helpers.py:942:24: note: Error code "type-arg" not covered by "type: ignore" comment
+ aiohttp/helpers.py:942:24: note: See https://mypy.rtfd.io/en/stable/_refs.html#code-type-arg for more info
+ aiohttp/helpers.py:945:26: error: "SimpleCookie" expects no type arguments, but 1 given  [type-arg]
+ aiohttp/helpers.py:1028:43: error: "SimpleCookie" expects no type arguments, but 1 given  [type-arg]
+ aiohttp/cookiejar.py:66:53: error: "SimpleCookie" expects no type arguments, but 1 given  [type-arg]
+ aiohttp/cookiejar.py:169:22: error: "SimpleCookie" expects no type arguments, but 1 given  [type-arg]
+ aiohttp/cookiejar.py:235:35: error: "SimpleCookie" expects no type arguments, but 1 given  [type-arg]
+ aiohttp/cookiejar.py:246:25: error: "SimpleCookie" expects no type arguments, but 1 given  [type-arg]
+ aiohttp/connector.py:228:23: error: "SimpleCookie" expects no type arguments, but 1 given  [type-arg]
+ aiohttp/client_reqrep.py:377:12: error: "SimpleCookie" expects no type arguments, but 1 given  [type-arg]
+ aiohttp/client_reqrep.py:710:23: error: "SimpleCookie" expects no type arguments, but 1 given  [type-arg]
+ aiohttp/web_request.py:574:17: error: "SimpleCookie" expects no type arguments, but 1 given  [type-arg]

starlette (https://github.com/encode/starlette)
+ tests/test_responses.py:347: error: "SimpleCookie" expects no type arguments, but 1 given  [type-arg]

srittau left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks! While the primer hits indicate that this change is disruptive, looking at the hits in depth indicates that the code in question can be simplified significantly. Currently, using SimpleCookie requires an explicit type annotation to specify the generic type (which can only be str), so I think the benefits outweighs the disruption significantly.

I'll leave it open for now if other maintainers want to chime in.

AlexWaygood commented Sep 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

I'm not too familiar with this module, so this may be a dumb question, but should even BaseCookie be generic? I can't find any uses on grep.app where BaseCookie is subscripted with anything other than str, and the docs don't shine any light on a situation where you might have a Morsel[bytes] or whatever as the value in a BaseCookie dictionary.

>>> from http.cookies import BaseCookie
>>> b = BaseCookie()
>>> b['foo'] = b'bar'
>>> b['foo'
<Morsel: foo=b'bar'>
>>> _.coded_value
"b'bar'"
>>> type(b['foo'].coded_value)
<class 'str'>

flaeppe commented Sep 12, 2023

Copy link
Copy Markdown
Author

Yeah, I don't know either. I'm getting a feeling it might be about conversion between formats rather than conversion to e.g. bytes. But I'm only guessing here

Copy link
Copy Markdown
Member

Chances are it's a Python 2 remnant: maybe in Python 2 you could have both str and unicode cookies.

Copy link
Copy Markdown
Member

Looks like both classes were originally made generic in:

But there's no explanation in that PR thread as to why

flaeppe commented Sep 12, 2023

Copy link
Copy Markdown
Author

Documentation for BaseCookie.value_encode says that coded_value will always "be converted to a string". Where it is set as Morsel.coded_value. So I suppose it should be valid to declare BaseCookie[int] and then implement a value_decode that parses an int from a str?

I'm not too familiar with this module, so this may be a dumb question, but should even BaseCookie be generic? I can't find any uses on grep.app where BaseCookie is subscripted with anything other than str, and the docs don't shine any light on a situation where you might have a Morsel[bytes] or whatever as the value in a BaseCookie dictionary.

>>> from http.cookies import BaseCookie
>>> b = BaseCookie()
>>> b['foo'] = b'bar'
>>> b['foo'
<Morsel: foo=b'bar'>
>>> _.coded_value
"b'bar'"
>>> type(b['foo'].coded_value)
<class 'str'>

I think an additional thing to check up on here is this

>>> from http.cookies import BaseCookie
>>> class C(BaseCookie):
...     def value_encode(self, value):
...         return value, value
...
>>> c = C()
>>> c["foo"] = b"bar"
>>> c["foo"]
<Morsel: foo=b'bar'>
>>> c["foo"].coded_value
b'bar'
>>> type(c["foo"].coded_value)
<class 'bytes'>
>>> c["foo"].output()
"Set-Cookie: foo=b'bar'"

Morsel.coded_value is always stringified before it's sent. At least that's what I interpret from "coded_value will always be converted to a string".

Funny thing is though, just because you've declared BaseCookie[int] it won't make .coded_value return int. And the only control over that is to enforce usage of Morsel[int] as set value.

Consider this program, which passes, but coded_value is <class 'str'>

from http.cookies import BaseCookie, Morsel
class C(BaseCookie[int]):
    ...

c = C()
c["foo"] = "1"
coded_value = c["foo"].coded_value
reveal_type(coded_value)  # Revealed type is "builtins.int"

But if you'd instead use below, types align.

m = Morsel[int]()
m.set("foo", "1", 1)
c["foo"] = m

AlexWaygood commented Sep 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

OK cool, sounds like BaseCookie should remain generic, then, in order to allow flexibility for hypothetical user subclasses of BaseCookie. Thanks for the research and the thorough explanation @flaeppe!!

AlexWaygood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

srittau merged commit c9bf034 into python:main Sep 13, 2023
flaeppe deleted the fix/simple-cookie-not-generic branch September 13, 2023 12:05
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Declare http.cookies.SimpleCookie as non generic(?)

4 participants


Back | FazBrowse Home | New Git URL