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
Support cookies with value None
#3096
base: master
Are you sure you want to change the base?
Changes from all commits
19ea7ff
441f3dd
a97599a
b505ce9
131731f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1010,7 +1010,7 @@ async def aclose(self) -> None: | |
await self.stream.aclose() | ||
|
||
|
||
class Cookies(typing.MutableMapping[str, str]): | ||
class Cookies(typing.MutableMapping[str, typing.Optional[str]]): | ||
""" | ||
HTTP Cookies, as a mutable mapping. | ||
""" | ||
|
@@ -1048,7 +1048,9 @@ def set_cookie_header(self, request: Request) -> None: | |
urllib_request = self._CookieCompatRequest(request) | ||
self.jar.add_cookie_header(urllib_request) | ||
|
||
def set(self, name: str, value: str, domain: str = "", path: str = "/") -> None: | ||
def set( | ||
self, name: str, value: str | None, domain: str = "", path: str = "/" | ||
) -> None: | ||
""" | ||
Set a cookie value by name. May optionally include domain and path. | ||
""" | ||
|
@@ -1080,23 +1082,26 @@ def get( # type: ignore | |
default: str | None = None, | ||
domain: str | None = None, | ||
path: str | None = None, | ||
raise_when_not_found: bool = False, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we avoid adding this API as part of the pull request? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here, I wanted to preserve this responsibility of raising the However, if it's safe to raise it whenever it's not found through What do you think? |
||
) -> str | None: | ||
""" | ||
Get a cookie by name. May optionally include domain and path | ||
in order to specify exactly which cookie to retrieve. | ||
""" | ||
value = None | ||
value = default | ||
found = False | ||
for cookie in self.jar: | ||
if cookie.name == name: | ||
if domain is None or cookie.domain == domain: | ||
if path is None or cookie.path == path: | ||
if value is not None: | ||
if found: | ||
message = f"Multiple cookies exist with name={name}" | ||
raise CookieConflict(message) | ||
value = cookie.value | ||
found = True | ||
|
||
if value is None: | ||
return default | ||
if not found and raise_when_not_found: | ||
raise KeyError(name) | ||
return value | ||
|
||
def delete( | ||
|
@@ -1141,14 +1146,11 @@ def update(self, cookies: CookieTypes | None = None) -> None: # type: ignore | |
for cookie in cookies.jar: | ||
self.jar.set_cookie(cookie) | ||
|
||
def __setitem__(self, name: str, value: str) -> None: | ||
def __setitem__(self, name: str, value: str | None) -> None: | ||
return self.set(name, value) | ||
|
||
def __getitem__(self, name: str) -> str: | ||
value = self.get(name) | ||
if value is None: | ||
raise KeyError(name) | ||
return value | ||
def __getitem__(self, name: str) -> str | None: | ||
return self.get(name, raise_when_not_found=True) | ||
|
||
def __delitem__(self, name: str) -> None: | ||
return self.delete(name) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @T-256 ! Actually, That was my initial attempt, but the test workflow failed with this notation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, no problem then...