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

Revision of the API based on Ralith's ideas. #9

Closed
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@jimmycuadra
Member

jimmycuadra commented Jan 28, 2017

Supercedes #6.

I talked with @Ralith some more tonight and the idea he's been trying to explain finally clicked. The key that I wasn't getting before is that ruma-api would be used by both ruma-client-api and ruma/ruma-client. Using this approach, ruma-api is still agnostic to any particular HTTP library, but ruma-client-api can still have a general concept of the components of a request.

Instead of the dependency graph looking like this:

ruma/ruma-client
       |
       V
ruma-client-api
       |
       V
   ruma-api

It would look like this:

ruma/ruma-client
  |    |
  |    V
  |  ruma-client-api
  |    |
  V    V
 ruma-api

ruma-api provides the Endpoint trait which has only two associated types: a request and a response. It also provides two structs, Request and Response, which are abstract representations of an HTTP request and response, not coupled to any HTTP library. Endpoint requires that its request and response implement conversions between their own types and these abstract types provided by ruma-api.

ruma-client-api implements Endpoint for all the endpoints in the client-server API. For each endpoint, it provides a single request struct and a single response struct as its public interface, so that consumers don't need to care about which part of the HTTP request or response a piece of data goes in. It then provides conversions between these two types, and ruma-api's abstract Request and Response.

ruma and ruma-client simply bind ruma-client-api with iron and hyper, respectively, by providing another set of conversions between iron/hyper's request/response and ruma-api's abstract Request and Response.

I haven't finished spiking this out, as you can see from the TODO comments and failing build, but I wanted to share and get some feedback and perhaps some advice for the remaining implementation.

Converting ruma_api::Request back into the concrete request type in the example in the docs feels awkward. In ruma, we use the router crate, which handles extracting the path parameters. I'm not sure how to get the path parameter in the implementation of TryFrom<ruma_api::Request> for Request in the example because we don't have that router middleware as input.

I'm also not sure what the associated Error type should be for the two TryFrom implementations.

/cc @Ralith @vberger @eternaleye @jplatte @exul @neosam

@jplatte

This comment has been minimized.

Contributor

jplatte commented Feb 1, 2017

I think I understand this idea better now that I've looked at it again and thought about it. We would basically have the request and response types we were previously planning to add to ruma-client in ruma-client-api, right?

I'd love to see this compiling in any case, just so I could write an implementation for one endpoint in ruma-client-api + ruma-client with it and see how it works. I'll share my experience with the experiment with the previous API proposal soon too (remind me on matrix if I forget).

@jimmycuadra

This comment has been minimized.

Member

jimmycuadra commented Feb 2, 2017

I finished implementing the example. Not as bad as I thought. I imagine we'll want to come up with a macro of some sort to reduce boilerplate, but maybe not.

Here's a direct link to viewing lib.rs so it's easy to read the code. I find it hard to see the new API clearly looking at a diff from master.

https://github.com/ruma/ruma-api/blob/e5b0280dfc7956f761fb164172d31c953418c9d8/src/lib.rs

@jimmycuadra

This comment has been minimized.

Member

jimmycuadra commented Feb 2, 2017

Is it worth it for Endpoint::info to return a static reference? It could also return an owned Info, which would make the implementation a little prettier.

@Ralith

This comment has been minimized.

Ralith commented Feb 4, 2017

It seems staggeringly unlikely that it will matter, so I'd just go with whatever's more convenient.

@jimmycuadra

This comment has been minimized.

Member

jimmycuadra commented May 12, 2017

Closing in favor of the API I just landed on master.

@jimmycuadra jimmycuadra deleted the ralith branch May 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment