-
-
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
Fix inconsistent exception type in response.json() method #5856
Conversation
… matter if simplejson is installed
Reverted some changes
Added changes for the latest version and mention backwards compatibility
Added latest changes to the changelog
@sigmavirus24 @nateprewitt I made the necessary edits to fix workflow failures. Any feedback on Python 2 compatibility before running the workflows again? Thanks. |
Hi @steveberdy, I'd recommend setting up a testing environment locally with our tox script and something like |
@nateprewitt Thank you for the advice. I ran the workflows, and all tests passed: https://github.com/steveberdy/requests/actions/runs/1023889042 |
Is this ready to merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates, @steveberdy. I added a couple of comments for discussion. I'm not convinced they're necessarily the right direction, but it does feel like the simplejson/json problems are potentially being spread more places than needed.
Let me know what you think, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I think this looks like we’re ready now. Thanks for seeing this through, @steveberdy.
For merging, I’m not sure if we want to put this on a 2.27.x branch while we wait for the next release, or if we merge now knowing we won’t do another patch release for 2.26.x. Any thoughts @sigmavirus24?
Let's just commit to the next release being 2.27.0. Heck we can push that out whenever and if we can get the proxy stuff merged too (once it's in a good state) then I think we can cut 2.27 with just these two "features" |
Alright, sounds like a plan to me. Thanks again, @steveberdy. |
Summary
Fixes the inconsistency of errors thrown in the
response.json()
method inmodels.py
, while preserving backwards compatibility.What we used to have
Depending on whether or not
simplejson
was installed in the user's library, and whether the user was running Python 3+ versus Python 2, a different exception would get thrown when there was an issue with decoding response text as JSON. Ifsimplejson
was installed,simplejson.JSONDecodeError
would get thrown. If not,json.JSONDecodeError
would get thrown for Python 3+ users, andValueError
for Python 2 users. Thus, depending on the scenario, users would find themselves having to catch eithersimplejson.JSONDecodeError
,json.JSONDecodeError
, orValueError
in the case of theresponse.json()
method failing. This inconsistency is not ideal.What we have now
There is now one error class in
exceptions.py
that will represent all errors that can be thrown or caught in theresponse.json()
method:requests.JSONDecodeError
. Allsimplejson
functionality was replaced withjson
functionality, but the new error type aliases bothjson.JSONDecodeError
andsimplejson.JSONDecodeError
. Ifsimplejson
is not installed, itsJSONDecodError
is replaced with a plainException
when aliased. If the user is running Python 3+,json.JSONDecodeError
is aliased, but if the user is running Python 2, that is replaced withValueError
when aliased, as theJSONDecodeError
was not previously a part of thejson
library. Now, all five error types,json.JSONDecodeError
,simplejson.JSONDecodeError
,ValueError
, andrequests.JSONDecodeError
and its parent classrequests.RequestException
, will be caught from therequests.JSONDecodeError
that will be raised.Fixes #5794
@sigmavirus24