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 [email protected]

Fix #2922

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

Xuanwo
Xuanwo commented Jul 26, 2022
View reviewed changes
@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
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

@seanmonstar seanmonstar Awaiting requested review from seanmonstar

1 more reviewer

@tomkarw tomkarw tomkarw left review comments

Reviewers whose approvals may not affect merge requirements
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