Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upStringify response before json.loads #91
Conversation
frewsxcv
reviewed
Sep 9, 2016
| @@ -141,6 +141,7 @@ def __init__(self, code, headers, **kwargs): | |||
| raw_response_body = self._raw_body | |||
|
|
|||
| try: | |||
| raw_response_body = str(raw_response_body) | |||
This comment has been minimized.
This comment has been minimized.
frewsxcv
Sep 9, 2016
Contributor
This probably doesn't need to be in the try, unless str(..) raises a ValueError, which I don't think it does? Not that big of a deal though.
This comment has been minimized.
This comment has been minimized.
AroundTheLines
Sep 9, 2016
Contributor
That was how I found the code, so I didn't change that. I did just change location of the stringify just now though.
AroundTheLines
added some commits
Sep 9, 2016
frewsxcv
reviewed
Sep 12, 2016
| @@ -141,6 +141,8 @@ def __init__(self, code, headers, **kwargs): | |||
| raw_response_body = self._raw_body | |||
|
|
|||
| try: | |||
| if not isinstance(raw_response_body, str): | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
frewsxcv
Sep 12, 2016
Contributor
Just thinking out loud, if it's a str only when zlib.decompress gets called (a few lines above this), we should maybe move the decode() up in that conditional.
This comment has been minimized.
This comment has been minimized.
AroundTheLines
Sep 13, 2016
Contributor
I'm not entirely sure if that's the case, I'll test it out and get back to you on that.
This comment has been minimized.
This comment has been minimized.
itsamaik
commented
Oct 18, 2016
•
|
Please merge this fix into the latest version, currently I'm also getting this error when retrieving job results. After zlib.decompress it's returning b'{ .......... }' thus resulting into an error when it's put into json.loads(). Edit: This seems to be an issue with Python 3+, see https://bugs.python.org/issue13989. TextIOWrapper could solve this. |
jbabichjapan
assigned
tushdante
Nov 22, 2016
This comment has been minimized.
This comment has been minimized.
jbabichjapan
commented
Nov 24, 2016
|
@AroundTheLines Hey could you check the latest feedback and let us know if you are ready to go with this change? I don't have time to help drive this one in right now but if you need more eyeballs please call out and hopefully some people will notice and help review |
This comment has been minimized.
This comment has been minimized.
|
[Edited for clarity: changes only local] As @frewsxcv suggested, I tested raw_response_body = zlib.decompress(self._raw_body, 16 + zlib.MAX_WBITS).decode()Async stats working locally for both Python 2.7.12 and Python 3.5.2. Any thoughts on this, @AroundTheLines? cc: @jbabichjapan, @tushdante |
This comment has been minimized.
This comment has been minimized.
|
@AroundTheLines: We'd like to merge this fix. Please let us know if you're able to make the aforementioned changes. If not, we can do it, too. Thanks for bringing this up and proposing a solution! |
This comment has been minimized.
This comment has been minimized.
|
@juanshishido: Hey, sorry I haven't been keeping up with this PR. I'd be glad to make the changes in a moment. |
AroundTheLines
added some commits
Dec 9, 2016
AroundTheLines
reviewed
Dec 9, 2016
Like @juanshishido said, decompressing the response at the source solves python 3 compatibility issues, where bytes are returned instead of strings.
This should be ready for merging!
cc: @jbabichjapan, @tushdante
juanshishido
merged commit 13d44f0
into
twitterdev:master
Dec 10, 2016
1 check passed
This comment has been minimized.
This comment has been minimized.
|
Awesome! Thanks, @AroundTheLines! |
This comment has been minimized.
This comment has been minimized.
madman-bob
commented
Aug 29, 2017
|
I'm having this issue sporadically in version 1.2.2. Eyeballing the source, I'm guessing we also need a Can anyone more knowledgable than I confirm/deny? |
AroundTheLines commentedSep 9, 2016
This fixes the "the json object must be str, not 'bytes'" error that happens on line 145.
Sample error before fix: