proposed 3.0 - altered internal loops of resolve_redirect

[jvanasco]

This is a resubmit of #3871 against the 3.0.0 branch.

[codecov-io]

Codecov Report

Merging #3886 into proposed/3.0.0 will decrease coverage by -0.02%.
The diff coverage is 100%.

@@                Coverage Diff                 @@
##           proposed/3.0.0    #3886      +/-   ##
==================================================
- Coverage           89.22%   89.21%   -0.02%     
==================================================
  Files                  15       15              
  Lines                1931     1928       -3     
==================================================
- Hits                 1723     1720       -3     
  Misses                208      208

Impacted Files	Coverage Δ
requests/sessions.py	93.04% <100%> (-0.08%)	❌

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b22833c...8e07dae. Read the comment docs.

[Lukasa]

I'm happy with this but would like extra review from @sigmavirus24. The fact that this counts as regressing code review is stupid but there we are.

[sigmavirus24]

The fact that this counts as regressing code review is stupid but there we are.

How do you mean, @Lukasa?

This looks generally fairly good, but I'm not sure I'm entirely clear. We're changing the ordering of the response history? Is there a good reason why? Has it been wrong all this time?

[jvanasco]

This looks generally fairly good, but I'm not sure I'm entirely clear. We're changing the ordering of the response history? Is there a good reason why? Has it been wrong all this time?

The original approach managed the history before a new request was made and ignored the original request. So it was "wrong" as it missed the original request. The logic on the loop's construction is somewhat painful to follow, and requires a lot of cleanup in send to build the correct history.

Assuming the URI a is requested and g is the final URI, the original code sets up history like this:

history = [b, c, d, e, f, g, ]

the send routine then cleans it up as such:

history.insert(0, a)
r = history.pop()
r.history = history

So the result is:

r = g
r.history = [a, b, c, d, e, f, ]

The new setup maintains the actual history:

history = [a, b, c, d, e, f, g, ]

So the cleanup in send is more straightforward:

r = history.pop()

and we end up with:

r = g
r.history = [a, b, c, d, e, f, ]

This patch makes the history less-wrong and removes the weird juggling that was required by the loop. The first element is now correct (the original a), however the last element of history is still the most-recent request/response and needs to be popped off to replace r. in the send method, r.history and history are distinct lists. r.history contains the actual history for that response (thanks to the fix in the above loop), however history is more like a list of the yielded/generated responses (that each contain a history).

[sigmavirus24]

Ah, I see. So this makes it better for folks handling their own redirect logic to see the complete history. 👍 Sounds good to me.

[jvanasco]

On Feb 25, 2017, at 10:05 AM, Ian Cordasco ***@***.***> wrote: Ah, I see. So this makes it better for folks handling their own redirect logic to see the complete history. 👍 Sounds good to me.

More importantly, It will make things much more straightforward for people subclassing "send" and future edits to the core redirect logic will be easier (I lost a few hours deciphering what was going on, because the variables swap around midway ).