-
Notifications
You must be signed in to change notification settings - Fork 26
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
Feature/bunq/sdk python#59 add response id to request error #64
Feature/bunq/sdk python#59 add response id to request error #64
Conversation
284cc29
to
a3ef206
Compare
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.
No plurals allowed 🙅🏾♂️
bunq/sdk/client.py
Outdated
@@ -215,7 +224,8 @@ def _assert_response_success(self, response): | |||
if response.status_code != self._STATUS_CODE_OK: | |||
raise ExceptionFactory.create_exception_for_response( | |||
response.status_code, | |||
self._fetch_error_messages(response) | |||
self._fetch_error_messages(response), |
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.
Plural
bunq/sdk/exception_factory.py
Outdated
_GLUE_ERROR_MESSAGES = '\n' | ||
_FORMAT_RESPONSE_ID_LINE = 'The response id to help bunq debug: {}' | ||
_FORMAT_ERROR_MESSAGE_LINE = 'Error message: {}' | ||
_GLUE_ERROR_MESSAGES_NEW_LINE = '\n' |
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.
Plural
bunq/sdk/exception_factory.py
Outdated
_FORMAT_RESPONSE_ID_LINE = 'The response id to help bunq debug: {}' | ||
_FORMAT_ERROR_MESSAGE_LINE = 'Error message: {}' | ||
_GLUE_ERROR_MESSAGES_NEW_LINE = '\n' | ||
_GLUE_ERROR_MESSAGES_STRING_EMPTY = '' |
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.
Plural
bunq/sdk/exception_factory.py
Outdated
|
||
return cls._glue_messages([line_response_code] + messages) | ||
return cls._glue_messages( |
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.
Plural
@epels all yours please 👀 |
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.
Few minor comments
@@ -39,6 +46,8 @@ class ApiClient(object): | |||
HEADER_GEOLOCATION = 'X-Bunq-Geolocation' | |||
HEADER_SIGNATURE = 'X-Bunq-Client-Signature' | |||
HEADER_AUTHENTICATION = 'X-Bunq-Client-Authentication' | |||
HEADER_RESPONSE_ID_UPPER_CASED = 'X-Bunq-Client-Response-Id' | |||
HEADER_RESPONSE_ID_LOWER_CASED = 'x-bunq-client-response-id' | |||
|
|||
# Default header values | |||
_USER_AGENT_BUNQ = 'bunq-sdk-python/0.12.4' |
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.
Don't forget bumping version
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 happens during release 🙃
|
||
@classmethod | ||
def create_exception_for_response(cls, response_code, messages): | ||
def create_exception_for_response( |
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.
Arg list not formatted according to PEP 8: https://www.python.org/dev/peps/pep-0008/#indentation
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.
correct, this is how we do it. The pep8 one is weird and ugly.
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.
Agreed 😝
response_code, | ||
messages, | ||
response_id | ||
) | ||
|
||
if response_code == cls._HTTP_RESPONSE_CODE_BAD_REQUEST: |
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.
Consider creating a mapping (response_code => exception type) for all these cases
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 code has not been modified in this pr, please create a follow up issue for this.
If so, this must also be refactored in the other SDK's
bunq/sdk/exception_factory.py
Outdated
""" | ||
:type response_id: str |
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.
Not ordered properly
bunq/sdk/exception_factory.py
Outdated
""" | ||
:type response_id: str |
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.
Not ordered properly
@@ -63,15 +109,21 @@ def _generate_message_error(cls, response_code, messages): | |||
|
|||
line_response_code = cls._FORMAT_RESPONSE_CODE_LINE \ | |||
.format(response_code) | |||
line_response_id = cls._FORMAT_RESPONSE_ID_LINE.format(response_id) | |||
line_error_message = cls._FORMAT_ERROR_MESSAGE_LINE.format( | |||
cls._GLUE_ERROR_MESSAGE_STRING_EMPTY.join(messages) |
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.
Consider whether _GLUE_ERROR_MESSAGE
would be sufficient as a name here. Having the name imply it's an empty string is redundant information and not relevant for the consumer IMHO
|
||
return cls._glue_messages([line_response_code] + messages) | ||
return cls._glue_all_error_message( | ||
[line_response_code, line_response_id, line_error_message] |
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.
No trailing comma?
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.
only in multi line
|
||
class TestPagination(BunqSdkTestCase): | ||
""" | ||
Tests if the response id from a failed request can be retrieved |
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.
Docstring should start on the same line as the opening """
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 also how we do it 😁 we ignore pep8 's suggestion to begin doctoring right behind """
""" | ||
""" | ||
|
||
caught_exception = None |
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.
|
||
self.assertIsNotNone(caught_exception) | ||
self.assertIsNotNone(caught_exception.response_id) | ||
self.assertIsNotNone(caught_exception.exception) |
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 assertion seems redundant after using assertRaises
?
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.
true
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.
LGTM
Closes #59