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.

Sign up for GitHub

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

Jump to bottom

Replace urllib with requests in openlibrary.core.fulltext #4410

Merged
merged 8 commits into from
Jan 13, 2021
Merged

Replace urllib with requests in openlibrary.core.fulltext #4410

merged 8 commits into from
Jan 13, 2021

Conversation

dherbst
Copy link
Contributor

@dherbst dherbst commented Jan 13, 2021

Addresses #2852

Refactor openlibrary.core.fulltext to replace urllib.openurl with requests.get. Adds unittests for the exception cases.

Technical

The fulltext search api function is used in a couple of places;

  • plugins/inside/code.py
  • plugins/worksearch/code.py

Testing

Unit tests were added for exception cases. You should also be able to do searches on staging in both "/search" and worksearch.

Screenshot

N/A

Stakeholders

@cclauss

cclauss
cclauss suggested changes Jan 13, 2021
View reviewed changes
Copy link
Contributor

@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.

Nice!

openlibrary/tests/core/test_fulltext.py Outdated Show resolved Hide resolved
openlibrary/core/fulltext.py Outdated Show resolved Hide resolved
openlibrary/core/fulltext.py Outdated Show resolved Hide resolved
openlibrary/core/fulltext.py Outdated Show resolved Hide resolved
@cclauss
Copy link
Contributor

cclauss commented Jan 13, 2021 •
edited
Loading

The other approach here would be to put the happy path in a single try block and handle the two exceptions like https://github.com/internetarchive/openlibrary/pull/4402/files

logger.debug('URL: ' + search_select)
try:
    response = requests.get(search_select, timeout=30)
    response.raise_for_status()
    return response.json()
except requests.HTTPError:
   a.b.c()
except JSONDecodeError:
    x.y.z()

@dherbst dherbst requested a review from cclauss January 13, 2021 19:02
cclauss
cclauss suggested changes Jan 13, 2021
View reviewed changes
Copy link
Contributor

@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.

json.decoder.JSONDecodeError will not be defined in Py2.

openlibrary/tests/core/test_fulltext.py Outdated Show resolved Hide resolved
openlibrary/tests/core/test_fulltext.py Outdated Show resolved Hide resolved
openlibrary/core/fulltext.py Outdated Show resolved Hide resolved
openlibrary/core/fulltext.py Outdated Show resolved Hide resolved
dherbst and others added 4 commits January 13, 2021 14:32
@dherbst @cclauss
Update openlibrary/tests/core/test_fulltext.py
fdf90b8 
Co-authored-by: Christian Clauss <cclauss@me.com>
@dherbst @cclauss
Update openlibrary/tests/core/test_fulltext.py
421a7a2 
Co-authored-by: Christian Clauss <cclauss@me.com>
@dherbst @cclauss
Update openlibrary/core/fulltext.py
7d410a0 
Co-authored-by: Christian Clauss <cclauss@me.com>
@dherbst @cclauss
Update openlibrary/core/fulltext.py
6cbc925 
Co-authored-by: Christian Clauss <cclauss@me.com>
@dherbst dherbst requested a review from cclauss January 13, 2021 19:37
@dherbst
Copy link
Contributor Author

dherbst commented Jan 13, 2021

Thanks, I applied those suggestions.

cclauss
cclauss approved these changes Jan 13, 2021
View reviewed changes
Copy link
Contributor

@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

@cclauss cclauss merged commit a0ae61d into internetarchive:master Jan 13, 2021
@dherbst dherbst deleted the 2852/refactor/replace-urllib-ol-core-fulltext branch January 13, 2021 20:48
Sabreen-Parveen pushed a commit to Sabreen-Parveen/openlibrary that referenced this pull request Feb 5, 2021
@dherbst @cclauss @Sabreen-Parveen
Replace urllib with requests in openlibrary.core.fulltext (internetar…
395b96a 
…chive#4410)

* Replace urllib with requests in openlibrary.core.fulltext

* Applied isort and black on fulltext and test_fulltext

* Simplify code by re-arraging try/except

* Update openlibrary/tests/core/test_fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update openlibrary/tests/core/test_fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update openlibrary/core/fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update openlibrary/core/fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>
Sabreen-Parveen pushed a commit to Sabreen-Parveen/openlibrary that referenced this pull request Feb 5, 2021
@dherbst @cclauss @Sabreen-Parveen
Replace urllib with requests in openlibrary.core.fulltext (internetar…
b6a8e0b 
…chive#4410)

* Replace urllib with requests in openlibrary.core.fulltext

* Applied isort and black on fulltext and test_fulltext

* Simplify code by re-arraging try/except

* Update openlibrary/tests/core/test_fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update openlibrary/tests/core/test_fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update openlibrary/core/fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update openlibrary/core/fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>
Sabreen-Parveen pushed a commit to Sabreen-Parveen/openlibrary that referenced this pull request Feb 5, 2021
@dherbst @cclauss @Sabreen-Parveen
Replace urllib with requests in openlibrary.core.fulltext (internetar…
b3c45c6 
…chive#4410)

* Replace urllib with requests in openlibrary.core.fulltext

* Applied isort and black on fulltext and test_fulltext

* Simplify code by re-arraging try/except

* Update openlibrary/tests/core/test_fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update openlibrary/tests/core/test_fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update openlibrary/core/fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update openlibrary/core/fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>
Sabreen-Parveen pushed a commit to Sabreen-Parveen/openlibrary that referenced this pull request Feb 5, 2021
@dherbst @cclauss @Sabreen-Parveen
Replace urllib with requests in openlibrary.core.fulltext (internetar…
050a3f8 
…chive#4410)

* Replace urllib with requests in openlibrary.core.fulltext

* Applied isort and black on fulltext and test_fulltext

* Simplify code by re-arraging try/except

* Update openlibrary/tests/core/test_fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update openlibrary/tests/core/test_fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update openlibrary/core/fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update openlibrary/core/fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>
Sabreen-Parveen pushed a commit to Sabreen-Parveen/openlibrary that referenced this pull request Feb 5, 2021
@dherbst @cclauss @Sabreen-Parveen
Replace urllib with requests in openlibrary.core.fulltext (internetar…
350168c 
…chive#4410)

* Replace urllib with requests in openlibrary.core.fulltext

* Applied isort and black on fulltext and test_fulltext

* Simplify code by re-arraging try/except

* Update openlibrary/tests/core/test_fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update openlibrary/tests/core/test_fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update openlibrary/core/fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update openlibrary/core/fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>
Sabreen-Parveen pushed a commit to Sabreen-Parveen/openlibrary that referenced this pull request Feb 15, 2021
@dherbst @cclauss @Sabreen-Parveen
Replace urllib with requests in openlibrary.core.fulltext (internetar…
146d9c6 
…chive#4410)

* Replace urllib with requests in openlibrary.core.fulltext

* Applied isort and black on fulltext and test_fulltext

* Simplify code by re-arraging try/except

* Update openlibrary/tests/core/test_fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update openlibrary/tests/core/test_fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update openlibrary/core/fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update openlibrary/core/fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>

* Update fulltext.py

Co-authored-by: Christian Clauss <cclauss@me.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cclauss cclauss cclauss approved these changes

Assignees

@cclauss cclauss

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dherbst @cclauss