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.
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.
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_redirectsloop.The existing code was hard to untangle as variables get overwritten midway through the loop (
respswitches between the previous and the next, which can be easy to miss). It manages thehistby omitting the first request, appending the previous request, then shuffling some stuff around in a cleanup during thesendmethod.In this approach, populating the
histis 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.historychanges, requiring a slightly different cleanup. The only code that callsresolve_redirectsis 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
sendis simplified and the context ofrespwhen 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.