ad hoc refactor in utilities/responses.py#9
ad hoc refactor in utilities/responses.py#90xNima wants to merge 2 commits intojobguywork:developfrom
Conversation
utilities/responses.py
Outdated
| class SuccessResponse(BaseResponse): | ||
| def __init__(self, data=None, message=None, show_type=settings.MESSAGE_SHOW_TYPE['TOAST'], status=200, **kwargs): | ||
| self.data = data | ||
| def __init__(self, message, data=None, **kwargs): |
There was a problem hiding this comment.
You are right. I get it wrong with ErrorResponse which message should be positional.
Also status should be checked to it's not be None.
utilities/responses.py
Outdated
| @staticmethod | ||
| def __make_response(status_code: int, extra: dict): | ||
| return { | ||
| 'success': True if status_code // 100 == 2 else False, |
There was a problem hiding this comment.
It's not a clean code, other programmers my have difficulty to read this section
There was a problem hiding this comment.
which part? __make_response method or only line 55?
There was a problem hiding this comment.
I think it's better to have a condition like 200 <= status_code < 300 it would be easier to understand the point of condition
There was a problem hiding this comment.
how about this:
from rest_framework import status
...
@staticmethod
def __make_response(status_code: int, extra: dict):
return {
'success': status.is_success(status_code),
'code': status_code,
'current_time': round(time.time()),
'show_type': settings.MESSAGE_SHOW_TYPE['NONE'],
**extra
}
utilities/responses.py
Outdated
| def __init__(self, message, data=None, **kwargs): | ||
| def __init__(self, data=None, **kwargs): | ||
| super().__init__(**kwargs) | ||
| self.message = message |
There was a problem hiding this comment.
Since it could be None, it's passed through kwargs and initialized in BaseResponse. Albeit maybe it's not good idea since the message field is initialized in each child class differently and inheritance is meaningless. When I moved the message initialization to BaseResponse, I thought that it could be None in each child class.
There was a problem hiding this comment.
Sorry I didn't see that, I need to test it manually because we have no test case, I will reply to you asap.
There was a problem hiding this comment.
this is not our expected result :)
There was a problem hiding this comment.
please test your code before any other commit or pull request
There was a problem hiding this comment.
Actually I can't load test data to my DB. I'll try again.
Thanks
2 participants
No description provided.