-
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
Changes from 10 commits
e036aef
7568934
e63aeb2
c2d288a
4817075
a727616
ef780ad
a3ef206
1041e0a
979b853
f3324e3
d5d92c6
177f605
aede581
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,40 +21,86 @@ class ExceptionFactory: | |
|
||
# Constants for formatting messages | ||
_FORMAT_RESPONSE_CODE_LINE = 'HTTP Response Code: {}' | ||
_GLUE_ERROR_MESSAGES = '\n' | ||
_FORMAT_RESPONSE_ID_LINE = 'The response id to help bunq debug: {}' | ||
_FORMAT_ERROR_MESSAGE_LINE = 'Error message: {}' | ||
_GLUE_ERROR_MESSAGE_NEW_LINE = '\n' | ||
_GLUE_ERROR_MESSAGE_STRING_EMPTY = '' | ||
|
||
@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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Agreed 😝 |
||
cls, | ||
response_code, | ||
messages, | ||
response_id | ||
): | ||
""" | ||
:type response_id: str | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not ordered properly |
||
:type response_code: int | ||
:type messages: list[str] | ||
|
||
:return: The exception according to the status code. | ||
:rtype: ApiException | ||
""" | ||
|
||
error_message = cls._generate_message_error(response_code, messages) | ||
error_message = cls._generate_message_error( | ||
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 commentThe 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 commentThe 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 |
||
return BadRequestException(error_message, response_code) | ||
return BadRequestException( | ||
error_message, | ||
response_code, | ||
response_id | ||
) | ||
if response_code == cls._HTTP_RESPONSE_CODE_UNAUTHORIZED: | ||
return UnauthorizedException(error_message, response_code) | ||
return UnauthorizedException( | ||
error_message, | ||
response_code, | ||
response_id | ||
) | ||
if response_code == cls._HTTP_RESPONSE_CODE_FORBIDDEN: | ||
return ForbiddenException(error_message, response_code) | ||
return ForbiddenException( | ||
error_message, | ||
response_code, | ||
response_id | ||
) | ||
if response_code == cls._HTTP_RESPONSE_CODE_NOT_FOUND: | ||
return NotFoundException(error_message, response_code) | ||
return NotFoundException( | ||
error_message, | ||
response_code, | ||
response_id | ||
) | ||
if response_code == cls._HTTP_RESPONSE_CODE_METHOD_NOT_ALLOWED: | ||
return MethodNotAllowedException(error_message, response_code) | ||
return MethodNotAllowedException( | ||
error_message, | ||
response_code, | ||
response_id | ||
) | ||
if response_code == cls._HTTP_RESPONSE_CODE_TOO_MANY_REQUESTS: | ||
return TooManyRequestsException(error_message, response_code) | ||
return TooManyRequestsException( | ||
error_message, | ||
response_code, | ||
response_id | ||
) | ||
if response_code == cls._HTTP_RESPONSE_CODE_INTERNAL_SERVER_ERROR: | ||
return PleaseContactBunqException(error_message, response_code) | ||
return PleaseContactBunqException( | ||
error_message, | ||
response_code, | ||
response_id | ||
) | ||
|
||
return UnknownApiErrorException(error_message, response_code) | ||
return UnknownApiErrorException( | ||
error_message, | ||
response_code, | ||
response_id | ||
) | ||
|
||
@classmethod | ||
def _generate_message_error(cls, response_code, messages): | ||
def _generate_message_error(cls, response_code, messages, response_id): | ||
""" | ||
:type response_id: str | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not ordered properly |
||
:type response_code: int | ||
:type messages: list[str] | ||
|
||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Consider whether |
||
) | ||
|
||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. only in multi line |
||
) | ||
|
||
@classmethod | ||
def _glue_messages(cls, messages): | ||
def _glue_all_error_message(cls, messages): | ||
""" | ||
:type messages: list[str] | ||
|
||
:rtype: str | ||
""" | ||
|
||
return cls._GLUE_ERROR_MESSAGES.join(messages) | ||
return cls._GLUE_ERROR_MESSAGE_NEW_LINE.join(messages) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
from bunq.sdk.exception import BadRequestException | ||
from bunq.sdk.model.generated.endpoint import UserPerson | ||
from tests.bunq_test import BunqSdkTestCase | ||
|
||
|
||
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 commentThe 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 commentThe 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 |
||
successfully. | ||
""" | ||
|
||
_INVALID_USER_PERSON_ID = 0 | ||
|
||
def test_bad_request_with_response_id(self): | ||
""" | ||
""" | ||
|
||
caught_exception = None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
try: | ||
UserPerson.get( | ||
self._get_api_context(), | ||
self._INVALID_USER_PERSON_ID | ||
) | ||
except BadRequestException as exception: | ||
caught_exception = exception | ||
|
||
self.assertIsNotNone(caught_exception) | ||
self.assertIsNotNone(caught_exception.response_id) |
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 🙃