-
Notifications
You must be signed in to change notification settings - Fork 151
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
feat: detailed error elated to API key and environment mismatch #391
base: master
Are you sure you want to change the base?
Conversation
@@ -8,9 +8,26 @@ class APIError(Exception): | |||
""" | |||
|
|||
def __init__(self, error, http_error=None): | |||
super().__init__(error) | |||
detailed_error = { | |||
"payload": 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.
sorry, please let me re-consider about compatibility with previous message
field.
alpaca/common/exceptions.py
Outdated
detailed_error["method"] = http_error.request.method | ||
detailed_error["url"] = http_error.request.url | ||
# add tips for auth key error | ||
if detailed_error["status_code"] in [401, 403]: |
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.
Do you need 403 here? If the URL is incorrect, you should check only 401?
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.
If I connect to live (sandbox=False) with a key for sandbox, it returns 403 as below. Therefore, it might be worth to include 403 ?
from alpaca.broker.client import BrokerClient
client = BrokerClient(api_key=BROKER_API_KEY, secret_key=BROKER_API_SECRET, sandbox=False)
client.list_accounts()
APIError: {'payload': '{"message": "forbidden."}\n', 'status_code': 403, 'reason': 'Forbidden', 'method': 'GET', 'url': 'https://broker-api.alpaca.markets/v1/accounts', 'tips': 'please check your API key and environment (paper/sandbox/live)'}
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.
If should be one of those. 403 is sad because API returns it for other cases as well such as not enough shares to sell. Downside of this approach is that the new error could be confusing in those cases unless you are really specific to the particular api auth error case
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.
Understood. Thank you for the info. In that case, agree, 401 only would be nicer. User still can get a hint by url strings.
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.
My question is, does api even return 401?
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.
Yes. API returns 401 when paper=False and use a key for paper as I wrote the first case in description.
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.
returns tips message only for 401. Addressed b7f027f
Context:
Changes:
Examples:
trading api
output
broker api
output