-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
hotfixing urllib2 + bwb API #2779
Conversation
LGTM |
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.
Although this has already been merged, I think we should circle back and clean it up so that it's more consistent.
@@ -2,6 +2,7 @@ | |||
import web | |||
import urllib2 |
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.
This should be removed and the exception handling for the referenced urllib2.HTTPError
reviewed and updated as necessary.
@@ -2,6 +2,7 @@ | |||
import web | |||
import urllib2 | |||
import simplejson |
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.
I'm suspicious of the simplejson
reference. Does the API really return data XML encoded and errors JSON encoded?
If the data is actually returned as XML, why are we "parsing" it with regexs instead of an XML parser?
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.
@mekarpeles I've been finding urllib2
references cause problems and can be replaced completely by requests
(as it looks like you have done below.
The simplejson
import is generally required with urllib2
to parse the response, but that is not needed with requests
as it has a built in json()
-- I think replacing urllib2
+ simplejson
with requests
everywhere in the code is something we should aim for, it helps with Python3 and just makes network requests so much easier. Not sure it warrants a single push effort, but I think making it known that it's a common goal may help refactoring + moving forward with adding new features.
The two imports on this file and refactor should be removed.
f = urllib2.urlopen(req) | ||
response = f.read() | ||
f.close() | ||
r = requests.get(url) |
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.
Note that Requests doesn't throw an exception like urllib2 does, so it needs explicit status checking.
I've taken a crack at repairing it in #2894, but this was flagged as an issue over 3 weeks ago, so I would have hoped to not have to take it on myself (as part of the Python 3 work).
A regression either on OL or BWB's side (relating to urllib2) is causing BWB API calls to fail. This PR is a
P0
hotfix which switchesurllib2
to userequests
incore/vendors.py
.Repro
Minimum repro case:
results in:
Test
https://dev.openlibrary.org/books/OL24658622M/Measuring_the_world has BWB price whereas https://openlibrary.org/books/OL24658622M/Measuring_the_world does not
Stakeholders
cc @bfalling, @cdrini