-
-
Notifications
You must be signed in to change notification settings - Fork 488
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
parameters.py - Expanding Error Code Catching #767
base: master
Are you sure you want to change the base?
Conversation
One public API I use returns errorCode: <error>. This should now catch a wider variety of errors.
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.
would be great if you also update the unit tests
If I have time this week I'll investigate your testing code. From Autodesk API an example error is: {'developerMessage': 'The required parameter(s) client_id not present in the request', 'errorCode': 'AUTH-008', 'more info': 'https://forge.autodesk.com/en/docs/oauth/v2/developers_guide/error_handling/'} So in parameters.py: if 'error' in params:
raise_from_error(params.get('error'), params) 'error' is not in |
Or rather I had a few moments now. Note I'm editing on GitHub and haven't cloned locally yet. So this might take a commit or two, |
Hi, I think the PR is too flexible as it does not enforce the list of field names. While the lowercase check might be a good idea (however a CaseInsensitiveDict would be easier to read?), the string search |
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.
We need to enforce the list of field names.
Change lowercase into a CaseInsensitiveDict.
The string search "error" in <string>
cannot work (acceptError
matches).
So if we had a predefined list of error codes? This way it follows RFC but can have added cases for API's that don't follow "standards" self.reportable_errors = ["error", "Error"]
def get_errors(self)
return self.reportable_errors
def add_error_code(self, err)
self.reportable_errors.append(err)
.
.
.
for key in params.keys():
if key in self.reportable_errors:
raise_from_error(params.get(key), params)
.
.
. |
Yes, that could work. However, I think the default's list should be what is in the RFC: only includes |
One public API I use returns errorCode: . This should now catch a wider variety of errors.