-
Notifications
You must be signed in to change notification settings - Fork 1
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
refactor: implementing abstract API class #74
Conversation
Classes: | ||
LikeDislikeUnitExperienceTestCase: Test LikeDislikeUnitExperienceView. |
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.
Nice catch
]) | ||
def setUp(self): | ||
"""Setup common conditions for every test case""" | ||
self.api_class = FuturexApiClient |
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.
These lines could be?
api_client = FuturexApiClient() |
api_client = FuturexApiClient() |
api_client = self.api_class()
return {} | ||
@property | ||
def base_url(self): | ||
return getattr(settings, "FUTUREX_API_URL") |
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 needs default value?
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, since in the past the decision was if someone wants to use it they must set up everything otherwise this would raise an exception
eox_nelp/api_clients/__init__.py
Outdated
response.status_code, | ||
) | ||
|
||
return {} |
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.
I know this is a refactor, but here you could a message of error or something related to the response...
How can I know if it is a 200 empty or an error? By the way, is the opportunity to handle it or maybe in the future because is working with that way now.
dc04f28
to
b8b3f65
Compare
This move the general logic from the FuturexAPIClient to a new Abstract class.
b8b3f65
to
a672ded
Compare
Description
This move the general logic from the FuturexAPIClient to a new Abstract class.