-
-
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
Create new Session method prepare_redirected_request
.
#1965
Conversation
This is almost entirely moving code around: the new method is most of the code that used to be the loop body of `resolve_redirects`.
Why are we removing the redirect mixin? |
It becomes obvious when you break up TBH I don't really understand why the mixin existed in the first place - it's not providing a useful conceptual division nor could it realistically have been applied to any other class but Session. |
(To be scrupulously accurate: the remaining use of internal Session data inside |
Testability (as was shown with #1963). Frankly, other parts of a Session would be easier to test if they were mix-ins as well (specifically the cookie handling pieces of logic). Then again, most people don't care how testable their code is.
That's not exactly right. Most people would not have wanted to apply it to any other class but Session, but it very realistically could have been applied to any other class that was carefully constructed (again, shown in #1963). |
I also disagree with your argument that it doesn't provide a good conceptual division, and I think this change emphasises the conceptual division, not weakens it. Redirection is complicated, and the number of methods that handle redirection is increasing all the time. The mixin provides an organisational framework for that code which makes the code easier to follow (IMO). The fact that you've added more methods here makes that even clearer. Yes, the mixin is tightly coupled to the |
Exactly what @Lukasa said — the Mixin is all about readability :) |
Closing due to inactivity. |
This is almost entirely moving code around: the new method is
most of the code that used to be the loop body of
resolve_redirects
.