-
Notifications
You must be signed in to change notification settings - Fork 41
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
Preserve original remote error attributes in API exception #159
base: master
Are you sure you want to change the base?
Conversation
What this fixes: - wild "except" statement - non-Pythonic direct type comparison (even worse, with "is") - textual FB response message is parsed and preserved; this may be handy when trying to automatically propagate a meaningful textual error reason somewhere upstream in the code (e.g. operator dashboard panel) - original HTTP status code is now preserved for easier debugging under the http_code attribute - original requests status exception is nicely chained (with the extended raise from statement)
9693da1
to
69e6e65
Compare
def __init__(self, message="Fireblocks SDK error", error_code=None): | ||
self.message = message | ||
def __init__(self, | ||
reason: str = '', |
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.
redundant
display_error = reason or \ | ||
(error_message and f"Got an error from fireblocks server: {error_message}") or \ | ||
"Fireblocks SDK error" | ||
|
||
super().__init__(display_error) |
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.
please fix without "reason" and make sure it is the exact same string as previous to preserve backward compatibility
check before what was the string if "response.text" was None
except (AttributeError, ValueError): | ||
response_data = None |
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.
what about other types of 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.
Which other types of exceptions do you have in mind that would be appropriate to silence and automatically handle this way?
As it is, the wild except statement would catch all exceptions, including system-related errors that have nothing to do with the Fireblocks library itself (e.g. program could be terminated, out of memory, etc..). In such cases it is more appropriate to propagate the exception rather than silencing it this way.
Wild except statements are very discouraged in general and are considered bad practice in Python.
What this fixes: