Skip to content

refactor: Remove Body's Once variant #2923

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 8 commits into from
Closed

refactor: Remove Body's Once variant #2923

wants to merge 8 commits into from

Conversation

Xuanwo
Copy link
Contributor

@Xuanwo Xuanwo commented Jul 26, 2022

Signed-off-by: Xuanwo github@xuanwo.io

Fix #2922

I'm not sure whether I correctly did this, PTAL.

Xuanwo added 2 commits July 26, 2022 10:32
@Xuanwo
refactor: Remove Body's Once variant
2120389 
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
Revert unintended code format
d974bfe 
Signed-off-by: Xuanwo <github@xuanwo.io>
Xuanwo
Xuanwo commented Jul 26, 2022
View reviewed changes
Xuanwo added 4 commits July 28, 2022 10:04
@Xuanwo
Add Body::Empty
cf99dce 
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
Make empty as inline
f415eab 
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
Cleanup not used functions
8dc4791 
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
Merge branch 'master' into remove-body
c4556a8
@Xuanwo Xuanwo requested a review from seanmonstar July 28, 2022 02:17
@seanmonstar
Copy link
Member

Looks like a couple tests that were using the Default impl will need to be changed to construct a Request manually.

@Xuanwo
Copy link
Contributor Author

Xuanwo commented Jul 29, 2022 

    pub fn get(&self, uri: Uri) -> ResponseFuture
    where
        B: Default,

Client::get needs B: Default, do we need to remove this API? (This will break many code)

@seanmonstar
Copy link
Member

No, that method doesn't need to be removed (yet). Just change the tests to use client.request(req).

Xuanwo added 2 commits July 30, 2022 19:04
@Xuanwo
Fix build
5c64501 
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
Merge branch 'master' into remove-body
c373b81
@Xuanwo
Copy link
Contributor Author

Xuanwo commented Jul 30, 2022

I'm feeling challenged to migrate tests like http2_detect_conn_eof. Can you give me some help?

In those tests, both request and response depend on hpyer::Body (so does Body::empty()), I can't replace them with http_body_util::Empty<bytes::Bytes> simplely.

Any ideas?

@seanmonstar
Copy link
Member

migrate tests like http2_detect_conn_eof.

I just took a look, and this should be able to fix it:

type Empty = http_body_util::Empty<Bytes>;

let service = service_fn(|_:Request<Body>| future::ok::<_, hyper::Error>(Response::new(Empty::new())));

// ...

let req = Request::builder()
            .uri(format!("http://{}/", addr))
            .body(Empty::new())
            .expect("request builder");

You don't need to change the Request<Body> part of the service_fn, since that's the request that hyper is generating.

@seanmonstar seanmonstar mentioned this pull request Aug 24, 2022
Merged
@seanmonstar
Copy link
Member

Thanks so much for taking it so far! I took care of merge conflicts and finishing out the examples, and completed it in #2922.

@Xuanwo Xuanwo deleted the remove-body branch August 25, 2022 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomkarw tomkarw tomkarw left review comments

@seanmonstar seanmonstar Awaiting requested review from seanmonstar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove Body's Once variant
3 participants
@Xuanwo @seanmonstar @tomkarw