-
Notifications
You must be signed in to change notification settings - Fork 169
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
Enhances how the SDK handles API error messages #338
Open
alessiocastrica
wants to merge
13
commits into
master
Choose a base branch
from
error-messages
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
36fe979
feat: bump websockets to latest version
alessiocastrica 38fac28
fix: del unused websocket
alessiocastrica 83b9aaf
draft: initial approach
alessiocastrica 2e55e74
fix: refactored APIError
alessiocastrica 2943165
fix: poetry lock
alessiocastrica 44e5058
fix: broken poetry lock after rebase
alessiocastrica 2537962
fix: black
alessiocastrica 5910e73
fix: workaround for doc check
alessiocastrica 1c5acd1
fix: remove workaround for doc check
alessiocastrica 6d49126
fix: formatting
alessiocastrica 1e84933
fix: lock
alessiocastrica e3ad16e
fix: comments
alessiocastrica 4d43e0b
fix: assert to appropriate exceptions
alessiocastrica File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -525,7 +525,7 @@ def _get_account_activities_iterator( | |
last_result = result[-1] | ||
|
||
if "id" not in last_result: | ||
raise APIError( | ||
raise AssertionError( | ||
"AccountActivity didn't contain an `id` field to use for paginating results" | ||
) | ||
|
||
|
@@ -658,19 +658,15 @@ def download_trade_document_for_account_by_id( | |
except HTTPError as http_error: | ||
if response.status_code in self._retry_codes: | ||
continue | ||
if "code" in response.text: | ||
error = response.json() | ||
if "code" in error: | ||
raise APIError(error, http_error) | ||
else: | ||
raise http_error | ||
raise APIError(http_error) from http_error | ||
|
||
# if we got here there were no issues', so response is now a value | ||
break | ||
|
||
if response is None: | ||
# we got here either by error or someone has mis-configured us, so we didn't even try | ||
raise Exception("Somehow we never made a request for download!") | ||
# we got here either by error or someone has mis-configured us, so we didn't even try | ||
assert isinstance( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this assert also can be replaceable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd raise a TypeError here, sounds good? |
||
response, Response | ||
), "Somehow we never made a request for download!" | ||
|
||
with open(file_path, "wb") as f: | ||
# we specify chunk_size none which is okay since we set stream to true above, so chunks will be as we | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,4 @@ | ||
import itertools | ||
import pprint | ||
from typing import Any, Dict, List | ||
|
||
import pandas as pd | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
built-in
AssertionError
is defined asRaised when an assert statement fails.
.Could be something different exception?
ref. https://docs.python.org/3/library/exceptions.html#AssertionError
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.
Maybe we can raise
ValueError
orAttributeError
? Which one it's better in your opinion? I'd go with the attribute error based on the docs