Collect history of responses if redirects occur.#614
Collect history of responses if redirects occur.#614asvetlov merged 5 commits intoaio-libs:masterfrom
Conversation
|
I support the PR but tests and documentation update should be added. |
|
I'll ... try? (I have never really done these things) |
|
Please do. I'll help you. |
|
Oh, I didn't see your comment until after I pushed the update. :-) |
|
Github doesn't send email notification on pushing. |
docs/client_reference.rst
Outdated
There was a problem hiding this comment.
Instead of "List" in "List of :class:ClientResponse objects …"?
Or where exactly do you mean?
There was a problem hiding this comment.
I mean
:class:`list` of :class:`ClientResponse` objects of preceding requests ...
|
Okay, I added a test. Edit: maybe add try/finally block around the asserts to release resp/resp_redirect? |
|
Perfect!
Please do. |
|
Done :-) |
|
Let me sleep on it and merge tomorrow. |
Collect history of responses if redirects occur.
|
Hi, yield from resp.history[0].text() |
|
Yes, all responses except the latest are released. |
|
I'm not quite sure what to do... |
|
Good catch. Converted |
|
If anything, I believe discarding the body and getting an exception when trying to access it should be the default behavior. I assume that the common case is, that you're not even interested in the body of preceding requests (if anything, you're probably more interested in the headers or the URLs - as in my case), and in most cases, it doesn't contain any valuable information anyway. So if you happen to be interested in the body of those requests, there should either be an option to completely load them or to somehow keep the connection opened, or to use |
Would you propose a patch? |
|
Hm, I'd have to take a closer look at how things work, but I would like to give it a try. As far as I can tell, awaiting .text() doesn't actually raise any exception but just returns an empty string on released or closed responses. Changing that would probably cause some applications, that rely on that, to break, and I don't know if it's worth to add an exemption just for redirects. I assumed it already raises some kind of exception - so I'm not sure after all, if it's a good idea. |
|
I don't care about |
|
We might as well just close the preceding responses instead of releasing them. If we do that, trying to read from |
|
No, response releasing is crucial for HTTP keep-alive support. Replacing Concurrent reading from different contents (bound with the same underlying transport) make a mess as @popravich mentioned. Honestly using just Did you get my point? |
Hi,
Accessing the history, or the information that a redirect occured at all (except for comparing original and final URL), is something I couldn't find and was missing from the requests library.
I'm completely new to this project, so I have no idea, if what I did, is the correct way to do it or if I missed something.
Feedback appreciated.