[bravado] Replace inbuilt async client with fido

[prat0318]

Side note: As fido exposes result() on future and swagger-py also exposes result(), changes to http_client are made to expose result() which internally calls wait().

It waits on Yelp/fido#2 to get merged in (some tests may be breaking right now).

PS: Not sure why flake8 are breaking in tests/mapping/schema/to_array_schema_test.py as it is untouched in this commit.

[prat0318]

This was added to unify interface of sync and async clients (as fido doesn't expose wait method).

[dnephin]

The interface definition is not really clear in the current version (it's more of a base class). I think the this interface needs a lot of work, it doesn't have to happen right now (but it should happen before the bravado release).

A major problem with the current implementation is that it's mutable. There is no reason for it to be.

I think it should be something like this:

RequestParams = namedtuple('RequestParams', ...)

class HttpClient(object):

    # this would replace start_request()
    def request(self, request_params):
        """Returns an HttpFuture"""

class HttpFuture(object):

    def result(self, timeout=DEFAULT_TIMEOUT):
        """Returns a swagger response, or raises an exception if there was an error making the request"""

[prat0318]

I will create a separate ticket for the refactoring.

Edit: Created #120

[analogue]

+1. I'd like to divorce any specific HttpClient implementation from bravado.mapping in preparation for splitting out to a separate python library (more details to come). Fido and Requests specifics should not be in bravado.mapping.

[prat0318]

@dnephin can you explain a bit on which part of the implementation is mutable here. I'm kinda lost about it.

[dnephin]

Hmm, I thought it used to be, but I don't see it any more, maybe it's been fixed already?

I guess the only issue now is the need for the branch logic, instead of passing along a Future that already has the correct adapter from the request() call.

[prat0318]

So, in operation.py L156 instead of returning the __call()__ as:

       return HTTPFuture(self.swagger_spec.http_client, request, response_future)

should the signature change to :

    return self.swagger_spec.http_client.request(request, response_future)

Which means, http_client's request() should now return back a HttpFuture object?

[dnephin]

Something like that, we should be able to delete handle_response from the module, so it would probably be more like:

return self.swagger_spec.http_client.request(request, self)

which lets us remove the response_future as well (it's just a closure so we have access to the operation anyway).

[analogue]

Github won't let me place this comment where I want (line 34 pre-commit, line 23 post-commit) but I think we need to stop beating around the bush and creating additional "naming" layers where they're not necessary. So:

s/AsynchronousHttpClient/FidoHttpClient/g
s/SynchronousHttpClient/RequestsHttpClient/g

Ok to defer to #120

[dnephin]

I like that, it'll make it easier to add other clients without name conflicts

[prat0318]

Will take it up in #120

[analogue]

Also, move HttpClient + friends into bravado.mapping or defer until #120.

lgtm

[dnephin]

move HttpClient + friends into bravado.mapping

I thought mapping was the package for things that are related to mapping swagger spec to a request , not related to the client?

[analogue]

@dnephin Ideally, I would like that to be the case too. However, I think it is going to be hard to decouple the two (mapping from the http client) w/o the interface for the http client residing in bravado.mappping.

Also, " + friends" means the non-concrete http client specific parts - the parts that define the interface only.

[dnephin]

Ah, I think I misunderstood originally. That seems fine (interface in mapping, implementation in bravado.http or something like that).