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

Replace urlopen with requests in openlibrary/views/showmarc #4527

Merged

Conversation

dherbst
Copy link
Contributor

@dherbst dherbst commented Feb 2, 2021

Related #2852

Refactor showmarc to replace urlopen with requests.

Technical

Refactor showmarc to replace urlopen with requests.
Some pylint and pycodestyle changes for formatting.

Testing

r0 = 0
r1 = 894
filename = 'CollingswoodLibraryMarcDump10-27-2008/Collingswood.out'
url = 'http://www.archive.org/download/%s' % filename
headers = {'Range': 'bytes=%d-%d' % (r0, r1)}
response = requests.get(url, headers=headers)
response.raise_for_status()

Screenshot

N/A

Stakeholders

@cclauss @hornc

@cclauss cclauss requested a review from hornc February 2, 2021 14:11
dherbst and others added 2 commits February 2, 2021 09:18
openlibrary/views/showmarc.py Outdated Show resolved Hide resolved
openlibrary/views/showmarc.py Outdated Show resolved Hide resolved
@dherbst dherbst requested a review from cclauss February 4, 2021 12:53
Copy link
Member

@mekarpeles mekarpeles left a comment

Choose a reason for hiding this comment

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

lgtm, over to @cclauss test + merge when you're able :)

@cclauss
Copy link
Collaborator

cclauss commented Feb 6, 2021

@hornc Do you approve?

@hornc
Copy link
Collaborator

hornc commented Feb 6, 2021

@cclauss The URLs still result in unnecessary redirects -- all archive.org URLs should be https: and without the www as per my earlier comment, but this will work with the requests change.

@dherbst
Copy link
Contributor Author

dherbst commented Feb 7, 2021

I must have overlooked the URL comment, sorry. I'll update it later tonight.

@dherbst dherbst requested review from hornc and cclauss February 7, 2021 02:35
Copy link
Collaborator

@hornc hornc left a comment

Choose a reason for hiding this comment

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

Thanks @dherbst ! @cclauss looks good to merge!

@cclauss cclauss merged commit 39a3835 into internetarchive:master Feb 7, 2021
Sabreen-Parveen pushed a commit to Sabreen-Parveen/openlibrary that referenced this pull request Feb 15, 2021
…archive#4527)

* Replace urlopen with requests.

* Update openlibrary/views/showmarc.py

Co-authored-by: Christian Clauss <[email protected]>

* Update openlibrary/views/showmarc.py

Co-authored-by: Christian Clauss <[email protected]>

* Update openlibrary/views/showmarc.py

Co-authored-by: Drini Cami <[email protected]>

* Update openlibrary/views/showmarc.py

Co-authored-by: Charles Horn <[email protected]>

* use https and remove www subdomain.

Co-authored-by: Christian Clauss <[email protected]>
Co-authored-by: Drini Cami <[email protected]>
Co-authored-by: Charles Horn <[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.

5 participants