-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
altered internal loops of how ``SessionRedirectMixin.resolve_redirect… #3871
Conversation
…s`` and ``Session.send`` handle redirect history.
Codecov Report
@@ Coverage Diff @@
## master #3871 +/- ##
=========================================
- Coverage 89.17% 89.07% -0.1%
=========================================
Files 15 15
Lines 1893 1894 +1
=========================================
- Hits 1688 1687 -1
- Misses 205 207 +2
Continue to review full report at Codecov.
|
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.
So I agree that this is a clarity improvement, but I am worried about the minor API changes. These are places that get overridden and subclassed a lot, and I have a real suspicion we'll break some stuff. I wonder if we should move this to the 3.0 branch instead.
requests/sessions.py
Outdated
@@ -97,17 +97,12 @@ def resolve_redirects(self, resp, req, stream=False, timeout=None, | |||
verify=True, cert=None, proxies=None, **adapter_kwargs): | |||
"""Receives a Response. Returns a generator of Responses.""" | |||
|
|||
hist = [] # keep track of history | |||
hist = [resp, ] # keep track of history; seed it with the original response |
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 need for the trailing comma in the list.
3.0 sounds good to me. I can remove the trailing comma. flake8 yells at me when i leave it out ;) |
What flake8 plugins are you using? That is not default behaviour. (Also, side note, Kenneth hates Flake8 with a passion ;)) |
updated, however I should probably do a different pr against the 3.0 branch instead -- right?
|
Yeah, you'd have to do a different PR against the 3.0 branch. |
closed in favor of #3886 for the 3.0 branch |
This is an idea for updating the
resolve_redirects
loop.The existing code was hard to untangle as variables get overwritten midway through the loop (
resp
switches between the previous and the next, which can be easy to miss). It manages thehist
by omitting the first request, appending the previous request, then shuffling some stuff around in a cleanup during thesend
method.In this approach, populating the
hist
is moved down the loop so the current request is topmost on the stack. The cleanup function is then simplified to just popping it off.This passes all tests, but causes a slight undocumented API change -- the value of
request.history
changes, requiring a slightly different cleanup. The only code that callsresolve_redirects
is withinSession.send
, but I don't know if this would break any hooks.anyways, I think this makes the block more straightforward as the "cleanup" function in
send
is simplified and the context ofresp
when appending the history doesn't change as much. another approach I thought of was using inline comments or variable name changes to clearly note which is the "previous_request" in that loop.