Refactore HttpClient and HttpFuture #127
Conversation
Conflicts: bravado/client.py bravado/mapping/operation.py docs/source/quickstart.rst tests/client_test.py
| return fido.fetch(url, **request_params) | ||
| fido_future = FidoFuture(op, url, request_params) | ||
| fido_future.fetch() | ||
| return fido_future |
There was a problem hiding this comment.
docstring and return type don't match. Also add op to docstring
|
lgtm except for the minor stuff |
| """ | ||
|
|
||
| def start_request(self, request_params): | ||
| def request(self, request_params, op=None): |
There was a problem hiding this comment.
Since operation is required, I would make it a regular argument, not a kwarg.
There was a problem hiding this comment.
This is because op is not required for api-docs fetch. For that instance, client.request(request_params) has to be called without op.
There was a problem hiding this comment.
Ah, ok, fair enough, kwargs works
|
This is looks good, I think with a few more small changes we can remove the mutability and inheritance as well |
|
|
||
| # op is not present during api-docs fetch, return raw response | ||
| if not self.op: | ||
| return response |
There was a problem hiding this comment.
I think having very different responses based on the presence of an operation is kind of unexpected.
I also just noticed that self.op is only being used the unmarshal the response.
It seems like this interface would be much more concise if unmarshal_response(response, op) were actually called by whatever calls this.
That would allow us to remove self.op as a parameter entirely.
There was a problem hiding this comment.
Ya, I think we can accomplish this by restoring handle_response(response, op), but without any of the branch logic. Since the adapters normalize the response to a single representation.
Conflicts: bravado/mapping/operation.py tests/mapping/operation/unmarshal_response_test.py tests/resource_response_test.py
@analogue I saw that in the integration tests, Also, functional tests fail with |
| :type response_adapter: :class: `bravado.mapping.response.ResponseLike` | ||
| :param callback: Function to be called on the response | ||
| """ | ||
| self.request_adapter = request_adapter |
There was a problem hiding this comment.
s/request_adapter/future/ , since the interface is concurrent.futures.Future
|
This looks great! Thanks for making all the changes. Just some minor rename/docstring stuff left |
|
@analogue I am adding |
| @@ -2,7 +2,7 @@ | |||
| # -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
looks like there are some remaining
There was a problem hiding this comment.
Removed the line: #!/usr/bin/env python.
Shouldn't we define the python encoding as utf-8 instead of default ascii as the best practice?
|
|
|
lgtm! good job |
Refactor HttpClient and HttpFuture
HttpClientandHttpFuture#120 (Most of the implementation taken from comments of [bravado] Replace inbuilt async client with fido #117)HttpClientandHttpFutureare place inbravado.mappingbravado/SynchronousEventualandhandle_responetestsPending: