Skip to content
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

2852/refactor/replace urllib with requests ol infobase #4419

Conversation

dherbst
Copy link
Contributor

@dherbst dherbst commented Jan 14, 2021

Addresses #2852

Refactor to replace simplejson with json, and urlopen with requests.get. The first two commits resolve the lint warnings and runs isort --profile black to reorder the imports.

Technical

Some reformatting to remove lint warnings.

Testing

Unit tests run, and can run the site using docker-compose up

Screenshot

N/A

Stakeholders

@cclauss

@cclauss
Copy link
Collaborator

cclauss commented Jan 14, 2021

Please do the black and isort thing only on brand new files.

Running these on existing files creates too much churn which makes changes too hard to see/review.

@dherbst
Copy link
Contributor Author

dherbst commented Jan 14, 2021

Ok - I'll hold off isort in the future. I did use three separate commits, so you could see only the simplejson/urllib changes in the last commit.

@dherbst dherbst requested a review from cclauss January 14, 2021 18:19
@cclauss cclauss self-assigned this Jan 14, 2021
Copy link
Collaborator

@cclauss cclauss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM...

urllib.request.urlopen().read() returns bytes and requests.get().text is str but I think we are OK with str here.

@cclauss cclauss merged commit b75996d into internetarchive:master Jan 18, 2021
Sabreen-Parveen pushed a commit to Sabreen-Parveen/openlibrary that referenced this pull request Feb 5, 2021
…ve#4419)

* Resolves lint warnings with reformatting, et al.

* Ran isort --profile black on ol_infobase

* Replace simplejson with json and urlopen with requests.get

* use more descriptive name, and add named param for requests.get

* Requests raise_for_status() needs to be called from a response

Co-authored-by: Christian Clauss <[email protected]>
Sabreen-Parveen pushed a commit to Sabreen-Parveen/openlibrary that referenced this pull request Feb 5, 2021
…ve#4419)

* Resolves lint warnings with reformatting, et al.

* Ran isort --profile black on ol_infobase

* Replace simplejson with json and urlopen with requests.get

* use more descriptive name, and add named param for requests.get

* Requests raise_for_status() needs to be called from a response

Co-authored-by: Christian Clauss <[email protected]>
Sabreen-Parveen pushed a commit to Sabreen-Parveen/openlibrary that referenced this pull request Feb 5, 2021
…ve#4419)

* Resolves lint warnings with reformatting, et al.

* Ran isort --profile black on ol_infobase

* Replace simplejson with json and urlopen with requests.get

* use more descriptive name, and add named param for requests.get

* Requests raise_for_status() needs to be called from a response

Co-authored-by: Christian Clauss <[email protected]>
Sabreen-Parveen pushed a commit to Sabreen-Parveen/openlibrary that referenced this pull request Feb 5, 2021
…ve#4419)

* Resolves lint warnings with reformatting, et al.

* Ran isort --profile black on ol_infobase

* Replace simplejson with json and urlopen with requests.get

* use more descriptive name, and add named param for requests.get

* Requests raise_for_status() needs to be called from a response

Co-authored-by: Christian Clauss <[email protected]>
Sabreen-Parveen pushed a commit to Sabreen-Parveen/openlibrary that referenced this pull request Feb 5, 2021
…ve#4419)

* Resolves lint warnings with reformatting, et al.

* Ran isort --profile black on ol_infobase

* Replace simplejson with json and urlopen with requests.get

* use more descriptive name, and add named param for requests.get

* Requests raise_for_status() needs to be called from a response

Co-authored-by: Christian Clauss <[email protected]>
Sabreen-Parveen pushed a commit to Sabreen-Parveen/openlibrary that referenced this pull request Feb 15, 2021
…ve#4419)

* Resolves lint warnings with reformatting, et al.

* Ran isort --profile black on ol_infobase

* Replace simplejson with json and urlopen with requests.get

* use more descriptive name, and add named param for requests.get

* Requests raise_for_status() needs to be called from a response

Co-authored-by: Christian Clauss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants