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.

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

bpross
Copy link

@bpross bpross commented Feb 25, 2018

PROBLEM

  1. 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.

  2. In their current form, unit tests are broken. The test_public_client.py file imports a library that is not listed in the requirements.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

  1. Implement error handling for public_client and authenticated_client requests.
  2. Require python-dateutil in requirements.txt. Send valid granularity.

CHANGES

  1. Created exceptions.py. Mapped all reported HTTP status codes from GDAX to appropriate exceptions.
  2. Created _determine_response method to raise appropriate exception or return JSON body.
  3. Created unit tests to verify error handling functions correctly.
  4. Added README information about error handling.
  5. Added python-dateutil to requirements.txt to fix unit tests.
  6. Changed granularity to 21600 instead of 10000, so unit test will pass.
  7. Bumped version to 2.0, as error handling will cause issues with implementations based on previous version.
  8. Added myself to contributors.txt

ETC
Addresses #245

@bpross
Copy link
Author

bpross commented Feb 25, 2018

@duffar12 @acontry @alex0527 @js931 please have a look

@alimcmaster1
Copy link
Contributor

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

@bpross
Copy link
Author

bpross commented Feb 25, 2018

@alimcmaster1 yes, I will make that change. I did not know those were defined in the PSL.

@danpaquin
Copy link
Owner

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
gdax/authenticated_client.py ~ Line 34

@bpross
Copy link
Author

bpross commented Mar 26, 2018

@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*
Copy link

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?

Copy link
Author

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.

@danpaquin
Copy link
Owner

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.

@danpaquin danpaquin closed this Aug 19, 2018
@danpaquin
Copy link
Owner

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?

@danpaquin danpaquin reopened this Aug 24, 2018
@bpross
Copy link
Author

bpross commented Aug 24, 2018

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.

@TheKeyboardKowboy
Copy link
Contributor

I would like to see error handling added.

@danpaquin
Copy link
Owner

I've hesitated on this since it's not backwards compatible.

Any thoughts from the community on this?

@py-pirate-readonly
Copy link

really like this. would like to see this PR updated for Coinbase-Pro, replacing "gdax" with "cbpro".

@bpross bpross force-pushed the add-error-handling branch from 537e1fb to 6d020e7 Compare November 30, 2018 02:51
@bpross
Copy link
Author

bpross commented Nov 30, 2018

Updated things since this got a little out of date.

@geegonzalez
Copy link

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).

@noah222
Copy link

noah222 commented Jun 2, 2019

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.

@lukastribus
Copy link

@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.

@vsai
Copy link

vsai commented Jan 3, 2022

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants