-
-
Notifications
You must be signed in to change notification settings - Fork 805
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
perf: using None checking cookies instead of bool(#2875) #2894
Conversation
httpx/_client.py
Outdated
@@ -397,7 +397,7 @@ def _merge_cookies( | |||
Merge a cookies argument together with any cookies on the client, | |||
to create the cookies used for the outgoing request. | |||
""" | |||
if cookies is None or self.cookies is None: | |||
if not (cookies is None or self.cookies is None): |
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.
This is not the same either... not (x or y)
is not the same is not x or not y
For example, x is true
and y is false
then not (x or y)
is false
and not x or not y
is true
.
Additionally, self.cookies
is always not None
so this will not work as intended
Line 183 in ae4d727
self._cookies = Cookies(cookies) |
httpx/_client.py
Outdated
@@ -397,7 +397,7 @@ def _merge_cookies( | |||
Merge a cookies argument together with any cookies on the client, | |||
to create the cookies used for the outgoing request. | |||
""" | |||
if cookies or self.cookies: | |||
if not (cookies is None or self.cookies is None): |
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.
if not (cookies is None or self.cookies is None): | |
if not (cookies is None and self.cookies is None): |
Then I think we can merge it after changelog added.
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.
No, we cannot. self.cookies
is always not None
.
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.
See https://github.com/encode/httpx/pull/2894/files#r1360817089
Marking this to avoid confusion re the review status.
Summary
Performance improve. Related #2875
Before
Now
Benchmark
Github Action Workflow test all passed.
Checklist