-
Notifications
You must be signed in to change notification settings - Fork 8
Suggestion: improve API exception handling in RequestHandler #15
Comments
Hi, I agree with what you're saying, the exceptions are too vague in many cases, and the approach you recommend could work better. I see 2 ways to improve exception handling:
OR
Let me know what you think, and thank you a lot for your input on this! |
Hi! |
raise_for_status will still raise a one-fits-all error HTTPError. It would be a better DX if the API returned different exception types for different errors. You don't have to handle all possible errors up front, but if there's a single place that parses the message, it would be easier to contribute new error types as we need them. |
Message when trying to buy more than available: |
proposal of exception hierarchy:
|
Hi,
First off, thanks for this nice little wrapper. Works great.
Here is my use case:
1 - List available loans
2 - Buy loans in a loop
This quickly causes the server to block me with a 429 response ("too many requests").
I want to be able to identify this response code and act accordingly (log it, wait and/or try to relogin).
As of now,
RequestHandler.request()
takes an Exception class as argument and raises an instance of that exception if the response code is => 400, no matter what the problem is.So at a higher level, the only way for me to find out what happened is to read the exception message. But this message comes straight from the server, and in the case of a 429, it is not very explicit and may not be specific to this error:
{'rules': 'wq7te'}
.Instead, I suggest creating and raising a TooManyRequests exception in
RequestHandler.request()
when the response code is 429. Or maybe there is a better way to handle it, such as usingrequests.raise_for_status()
.In addition to that, for my use case, the
InsufficientFunds
exception that is passed bypurchase_loan()
toRequestHandler.request()
covers too many cases. I counted at least 2 cases that cause this exception to be raised while my funds are sufficient, but there may be more:I'm not sure what the best solution would be. I would either remove the
exception_type
argument fromRequestHandler.request()
, only raise "technical" exceptions inRequestHandler.request()
and let higher level functions process the response (and let them raise "business" exceptions).I can drop a PR but would like to know your opinion beforehand.
The text was updated successfully, but these errors were encountered: