Skip to content
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

Add msgpack support #214

Closed
AlstonLin opened this issue Nov 2, 2017 · 2 comments
Closed

Add msgpack support #214

AlstonLin opened this issue Nov 2, 2017 · 2 comments

Comments

@AlstonLin
Copy link
Contributor

AlstonLin commented Nov 2, 2017

msgpack would be a more efficient way to receive large JSON body responses, and could mean large performance gains for people who want to use bravado with a server that supports application/msgpack responses

Support for this content type should be added to bravado

@AlstonLin
Copy link
Contributor Author

@macisamuele For this functionality, one of the things I'd need to do is to add msgpack unmarshalling in bravado_core. However, to be able to do that I need to have access to the bytes of a response, which is currently not possible under the interface specified in bravado_core.response.IncomingResponse. So to implement this, I'd need to change IncomingResponse to have 'bytes' in the required_attrs, which would be a breaking change as now all implementations of this, including the RequestsResponseAdapter and FidoResponseAdapter in bravado will need to be updated to use the latest version of bravado_core if pushed. This means that this feature would require a breaking change to bravado_core. Would you say this is a feature worth having a breaking change?

@macisamuele
Copy link
Collaborator

@AlstonLin this is just to recap our quick chat 😄
We can modify IncomingResponse in order to expose bytes other than text and json, it should not be a breaking change but according to how it will be implemented, we can decide whenever is safe to go with a minor version bump or a major version bump.

I checked bravado-core and bravado implementations and those are the points that I would focus (it doesn't represent a limitation to you, it's more about giving ahead places to look in order to overcome the 9 hours gap):

I'm pretty sure that I'm still missing something, but surely you'll figure it out 😄

sjaensch added a commit that referenced this issue Nov 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants