hyper icon indicating copy to clipboard operation
hyper copied to clipboard

feat: create Service trait

Open tomkarw opened this issue 1 year ago • 2 comments

Closes https://github.com/hyperium/hyper/issues/2853.

tomkarw avatar Jul 23 '22 21:07 tomkarw

Ok, I see. You could leave the tower-service dependency in for now, since the work to transfer this client stuff is in a separate PR, and then this PR is just replacing the server side. Then when both PRs are done, we can chop out of the Cargo.toml. I think that'll be the least complicated as all this movement happens.

seanmonstar avatar Jul 27 '22 23:07 seanmonstar

I might be doing something wrong, but it's impossible to just replace the server part. It would require maintaining duplicates of most parts of src/service and some parts src/common, one with stuff that implements tower_service::Service and one with hyper::service::Service. The clean sweep throughout the repo was the closest I got to compiling, working code. Trying to do anything local to src/server just keeps leaking to other parts of the repo.

@seanmonstar I could use some guidance here. I saw you did some work on removing tcp feature and parts of src/client. Should I maybe wait till client part is completely gone? I think clean sweep remove of tower_service dependency is the best way to move this MR forward.

tomkarw avatar Aug 01 '22 21:08 tomkarw

Alright, with the help of cross-cutting removals of code Sean have done to the client module, this PR was way easier to do.

Looks good to me now, @seanmonstar I'd ask for CI run and if it's all green (except for miri) a review.

tomkarw avatar Aug 18 '22 19:08 tomkarw

Dumb mistake, I fought the compiler for so long I forgot to run the tests once the project finally compiled.

Locally, all the tests (including examples) pass now.

There is only one reference to tower left in the whole project, and it's in client/conn/mod.rs here. We could easily make SendRequest implement hyper::service::Service instead and remove the dependency all together. It would be a clean sweep of tower/tower-service from the project.

tomkarw avatar Aug 19 '22 14:08 tomkarw

I can do fmt + check or test, but I seem to be uncapable of remembering to run all 3 of them before pushing :smile:

tomkarw avatar Aug 19 '22 15:08 tomkarw

Alright, couldn't stop myself from removing the last tower reference from client::conn::SendRequest. The repo is now all free of the dependency. If you think that's wrong, I can revert the last commit.

I'm certain pipelines will pass, so I'll wait for the decision and CR.

tomkarw avatar Aug 19 '22 18:08 tomkarw

Juuust one more CI trigger.

tomkarw avatar Aug 19 '22 18:08 tomkarw

@seanmonstar Could you address https://github.com/hyperium/hyper/pull/2920#discussion_r950670503 and https://github.com/hyperium/hyper/pull/2920#discussion_r950680044, and I'll move this PR forward?

tomkarw avatar Sep 01 '22 21:09 tomkarw

@seanmonstar I introduced changes from the discussion threads.

LGTM, let's wrap and merge this as soon as possible, as the merge conflicts on this branch are killing me :stuck_out_tongue_winking_eye:

tomkarw avatar Sep 05 '22 21:09 tomkarw

CI failed on a doc-test, this should be fixed now.

tomkarw avatar Sep 07 '22 00:09 tomkarw

Thanks so so so much! I know this dragged on a while, but it turns out there were tricky things for us to figure out (and conflicts as all sorts of other things got yanked).

seanmonstar avatar Sep 08 '22 22:09 seanmonstar