-
-
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
proposed 3.0 - altered internal loops of resolve_redirect #3886
Conversation
…s`` and ``Session.send`` handle redirect history. 3.0.0 branch
Codecov Report
@@ 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
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.
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.
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? |
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 Assuming the URI
the send routine then cleans it up as such:
So the result is:
The new setup maintains the actual history:
So the cleanup in
and we end up with:
This patch makes the |
Ah, I see. So this makes it better for folks handling their own redirect logic to see the complete history. 👍 Sounds good to me. |
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 ).
|
This is a resubmit of #3871 against the 3.0.0 branch.