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

Handle errors effectively #140

Closed
wants to merge 7 commits into from
Closed

Handle errors effectively #140

wants to merge 7 commits into from

Conversation

kumiDa
Copy link
Collaborator

@kumiDa kumiDa commented Mar 21, 2019 •
edited
Loading

This PR closes #139

requests.exceptions based are handled with a prompt.

Additional commits include:

  • Normalizing the naming of response variables in the module.
  • Updating the README.md with updated content of the CLI prompt.

@kumiDa
Copy link
Collaborator Author

kumiDa commented Mar 21, 2019 •
edited
Loading

I have made simple amends to the error handling as a start to completely improving the whole process.
Comments and suggestions to the actual error handling process are most welcome.
I believe the UX of this API wrapper should be of at most importance to when throwing these prompts.
So please point out the things to consider in achieving this.
cc: @hornc, @mekarpeles

hornc
hornc reviewed Mar 21, 2019
View reviewed changes
olclient/openlibrary.py Outdated
@@ -135,9 +148,20 @@ def save_many(self, docs, comment):
@backoff.on_exception(on_giveup=err, **BACKOFF_KWARGS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

err = lambda e: logger.exception("Error retrieving OpenLibrary response: %s", e)
@rahul-kumi This is the exception that the backoff decorator eventually raises/d -- I think we want this to give a clearer idea of what happened, whether it was an 'ID not found', timeout, server error, or something else. I thought the intent of #139 was to avoid raising generic exceptions. The requests exceptions are generally pretty informative, but I think this original code lost some of that information. The messages you have added are good and informative, but I think they need to be encapsulated as custom exceptions (rather than prints) so they can be handled further up the chain.

@hornc
Copy link
Collaborator

hornc commented Apr 23, 2019

Hi @rahul-kumi , where are we with this PR? Are you able to take a look at removing the print statements from the core client exceptions?

@kumiDa
Copy link
Collaborator Author

kumiDa commented Apr 24, 2019

Hi @rahul-kumi , where are we with this PR? Are you able to take a look at removing the print statements from the core client exceptions?

I have been busy for quite some time now. I shall take a look at this now.....

@mekarpeles
Copy link
Member

@hornc + @rahul-kumi -- this PR has been open for a bit, is there much work remaining?

@hornc hornc closed this Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hornc hornc hornc left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve Error handling in olclient/openlibrary.py
3 participants
@kumiDa @hornc @mekarpeles