-
Notifications
You must be signed in to change notification settings - Fork 732
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 error handling, fix broken unit tests #260
base: master
Are you sure you want to change the base?
Conversation
Firstly thanks for a well defined PR and for adding the test dependency, I missed this here #207. Changes generally look good to me at a first glance. One comment I would make those is in "public_client.py" do we have to define all the HTTP Status codes? Might be neater to use standard library such as https://docs.python.org/3/library/http.html, instead of declaring these? Thanks |
@alimcmaster1 yes, I will make that change. I did not know those were defined in the PSL. |
This is great. It looks like there are a few oversights, but other than that this looks solid. I will make these changes myself if this is left for another week. gdax/exceptions.py ~ Line 31 |
1b5c9d8
to
6685195
Compare
@danpaquin updated based on your concerns. Please let me know if there is anything else you want me to change. Cheers. |
## Change Log | ||
*2.0* |
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.
Why are you bumping the version to 2.0, is it not backwards compatible anymore?
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.
This is not backwards compatible, as it will raise exceptions instead of returning a response object.
Because this will cause braking changes, I believe we will need to table this feature for future use. I would rather push this into a separate branch as some users may not prefer this implementation. |
There was a lot of good work done here so I'm reopening this to gauge interest in this error handling implementation. How does the community feel about error handling in general? Would you find this useful? |
I definitely find it useful and am using it in my current setup. Let me know if we want to move forward with this and I can fix the rebase issues and make any improvements that the community would see fit. |
I would like to see error handling added. |
I've hesitated on this since it's not backwards compatible. Any thoughts from the community on this? |
really like this. would like to see this PR updated for Coinbase-Pro, replacing "gdax" with "cbpro". |
…X. Updated to check errors in send_message instead of _get
537e1fb
to
6d020e7
Compare
Updated things since this got a little out of date. |
Would love better error handling built into the library. Error handling is such an essential thing when working with API's. Because of the current limitations I copy/pasted the library into my own project and hacked in error handling of my own (not very pretty, I know). |
I don't think the code here is being maintained anymore, but it is very fast already. The best way to handle errors is with a try, except block surrounding each api call. This will allow you to catch each error and deal with it based on your specific needs. In the past 30 days I have traded many thousands with this method, so it is stable. |
@danpaquin I understand your reluctance to break the API, but I believe this is absolutely necessary. The library currently doesn't even raise an exception on simple authentication failures. It's very painful to debug strange behavior in your application, only to discover that a trivial authentication issue is causing this, because the library hides all errors. |
@danpaquin Would definitely like to see error handling implemented. There are several errors that can come about based on API restrictions and limitations, and without it, we're hesitant about the robustness of our usage of this API. For people to migrate their code is relatively simple, assuming they just replace all references of response with response.json() / response.text. |
PROBLEM
Currently this library does not support error handling for HTTP requests. This causes library clients to implement error handling on their end. A big problem with this is that clients do not have access to the response object, so clients are prevented from checking the status code. The only method for checking error states is to look for
message
in the response body.In their current form, unit tests are broken. The
test_public_client.py
file imports a library that is not listed in therequirements.txt
file. If running tests in a virtualenv, py.test will fail to collect the tests. Additionally,test_get_historic_rates
sends a request to GDAX with an invalid granularity, which causes a 400 to be returned.SOLUTION
python-dateutil
inrequirements.txt
. Send valid granularity.CHANGES
exceptions.py
. Mapped all reported HTTP status codes from GDAX to appropriate exceptions._determine_response
method to raise appropriate exception or return JSON body.python-dateutil
torequirements.txt
to fix unit tests.ETC
Addresses #245