-
-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add API exception #113
Add API exception #113
Conversation
Added the API exception alongside cleaned up a few area that have been made redundant
odins_spear/requester.py
Outdated
@@ -1,5 +1,6 @@ | |||
import requests | |||
import json | |||
from . import exceptions |
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.
Change to ' from .exceptions import OSAPIResponseError
When importing its good to keep them as light as possible getting only what you need.
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.
Just a few bits to touch up.
odins_spear/requester.py
Outdated
try: | ||
response.raise_for_status() | ||
except requests.exceptions.RequestException: | ||
raise exceptions.OSApiResponseError(response) |
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.
Adjust inline with first comment.
@@ -92,6 +93,12 @@ def _rate_limited_request(self, method, endpoint, data=None, params=None): | |||
self.logger._log_request(endpoint=endpoint, response_code=response.status_code) | |||
|
|||
# flags errors if any returned from the API | |||
response.raise_for_status() | |||
return response.json() | |||
try: |
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.
Add this to the the _request method. Its only applied to the rate limited version.
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.
Resolved comments
Added the API exception alongside cleaned up a few area that have been made redundant
This allows for more descriptive error handling when a request exception occurs
Previous:
New: