-
-
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
The iterable produced by Session.resolve_redirects
does not include the very first response
#1953
Comments
It seems like the best approach is the second with a better method name. |
What do you think a better method name would be? I am chronically bad at names. |
Heh, there's not really a good one here, but could be |
Wouldn't |
I thought about it, but both are misleading, as they contain the first response (which is not a redirect). Could be Anyway, this is bikeshedding of the highest order. The question is are we happy to add this method to the API? |
It's the very last response in the sequence that's not a redirect, but yeah, one of them isn't. I rather like |
I like All things considered, the goal here is more to provide an equivalent to On a side note, allow me to play devil's advocate. I can easily see people complaining that we only allow for this handling of redirects on such a low level. People will want this change to bubble up to |
Taking @sigmavirus24's concern on board, is there an elegant way we can do this outside of the library, e.g. in the toolbelt? |
I'm not familiar with this "toolbelt"? But, an alternative would be a
Since backward compatibility dictates preserving Having said that, personally I'm not much concerned about the feature creep issue, because I think anyone who wants to do manual redirection chasing is going to want to work with the Session API anyway. If nothing else, you probably need Session-level control over cookies. |
It isn't a very strong concern. It's more of a pattern I've seen develop as of late. People watch the repo for a tiny change and use that change to get their foot in the door for a larger one that is widely unnecessary. It's a tiny concern that's ever present now. Likewise, I think the toolbelt could easily accomodate this. That said, I'm not convinced it should be either in or outside of the core (i.e., I don't actually know where it belongs). I've also been thinking along the same lines @zackw, but more geared towards making an eventual refactor a lot easier. I like having a compliment to |
I can get behind that idea, though I'm +0.5 until I see some code. |
I really want to get some work done on the toolbelt and betamax today. If @zackw has the time to throw together an example of |
@sigmavirus24 Not a problem - it's a simple matter of moving code around. See #1965. (I am going to be offline for most of the rest of the day, though.) |
It's the weekend. Enjoy your Sunday! 🍰 |
If you are manually walking over redirects, you probably want to structure your code like this:
The existing API does not let you do that. You must write either
which involves writing the same "do something with" code in two places, or
which is un-Pythonic loop structure.
Since
Session.resolve_redirects
must remain as is for compatibility's sake, the only way to fix this is to add either a new mode tosend
(allow_redirects=MANUAL
?) or a new Session method (perhaps in fact calledsend_with_manual_redirect_walking
) which returns an iterable that does include the very first response. I do not particularly care which, or what the new method is called in the second case.The text was updated successfully, but these errors were encountered: