Refactor client response decoding #98
Conversation
|
I like the pull request. |
|
|
||
| return payload | ||
| return json.loads(self._content.decode('utf-8')) |
There was a problem hiding this comment.
- Please try to use encoding from Content-Type charset if specified.
- Please support encoding keyword-only method argument for overriding json encoding.
There was a problem hiding this comment.
Please support encoding keyword-only method argument for overriding json encoding.
I'm afraid, this isn't actual any more:
https://docs.python.org/3/library/json.html#json.loads
The other arguments have the same meaning as in load(), except encoding which is ignored and deprecated.
There was a problem hiding this comment.
I mean
return json.loads(self._content.decode(encoding))
not
return json.loads(self._content.decode('utf-8'), encoding)
There was a problem hiding this comment.
encoding passes as response.json(encoding='utf-8') and overrides defined in Content-Type header, correct?
|
I don't like close parameter. Both read() and json() methods are high level and both fully consume response payload and I think it is safe just release connection. On the other hand in close() if payload is not consumed we have to force close connection. |
|
@fafhrd91 I'd like to agree about close parameter - it's ugly and missing it may lead to connections leak. So remove it and always release connection after payload read? |
|
Yes, let's release connection implicitly. In any case it is still possible to manually read response from content object. |
|
Agree with implicit releasing connection. |
|
I think we also need release() coroutine In case if response doesn't matter. It can just read payload and release connection. |
|
Hm...good idea about One more question: could you suggest a better place for |
|
helpers.py? We can move all helper functions to this module.— On Thu, Jul 3, 2014 at 7:29 AM, Alexander Shorin notifications@github.com
|
oops, sorry, I push changes before read your response. Anyway, PR needs in rebase and squash since I made a mistake with logging - it's not available any more. UPDATE: helpers or utils? |
Deprecate ClientResponse.read_and_close method
Useful, when response body isn't available for read (HEAD requests) or if you're not interested in it, but wants release connection correctly.
There are several issues with current decoding implementation inside read method: - issue #18 - no possibility to use different json lib - doesn't scale for different mime types - it crushes for empty body - on call read(True) with decoding support for multiple types you won't be sure which kind of response you'll get back TL;DR there are too many reasons to patch .read() method because of decoding logic inside. Moving it to standalone method simplifies life a lot.
|
Rebased PR and squashed commits. |
|
very good! thanks. |
Refactor client response decoding
|
@kxepal Sure! |
On the rights of the idea. I'd used these changes for some time and found them very handy, would like to hear what you think about. While I expect that they could be a bit radical, current JSON handling inside
.read()is really creates more problems than tends to solve.Replacing
read_and_close()withread(close=True)is a necessary evil - otherwise we'll havejson()/json_and_close()etc. - that is awkward.