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

Bring response in line with http-types #562

Merged
merged 6 commits into from
Jun 1, 2020
Merged

Conversation

yoshuawuyts
Copy link
Member

@yoshuawuyts yoshuawuyts commented May 29, 2020

This is a follow-up to #537. Brings Response in line with Request. Thanks!

Changes

  • Removed Response::redirect in favor of tide::Redirect
  • Renamed Reponse::set_cookie to Response::insert_cookie.
  • Various Response methods no longer return Self.
  • Response::new now accepts u16 as well as StatusCode as arguments.
  • Added Response::header_mut
  • Renamed Response::set_header to Response::insert_header.
  • Removed Response::{body_string, body_form, body_json, body} in favor of Response::set_body.
  • Added Response::swap_body.
  • Renamed Reponse::set_ext to Response::insert_ext.

Copy link
Member

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good - guess I didn't have in my brain what all had happened in http_types last week

src/response.rs Outdated Show resolved Hide resolved
///
/// The content type is set to `text/plain; charset=utf-8`.
#[must_use]
pub fn body_string(mut self, string: String) -> Self {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not so sure about removing these - are we depending on always mime sniffing now? There's definitely some overhead if that's the case.

Otherwise the docs should probably be clear that the user needs to set it themselves.

Copy link
Member Author

@yoshuawuyts yoshuawuyts May 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not so sure about removing these - are we depending on always mime sniffing now? There's definitely some overhead if that's the case.

These methods have been moved to rely on Body instead and go through the set_body method instead. Mime sniffing is only applied for Body::from_file -- we set default mime types when creating a body from string, from json, or from bytes -- but in the most cases people will need to set it themselves. The set_body method covers most of the ground the previous methods used to:

// old API
res.body(io::Cursor::new(b"hello world".to_owned()));
res.body_string("hello world".to_string());

// new API
res.set_body(b"hello world");
res.set_body("hello world");

The only set of APIs that become a bit more awkward by this are the Serde-related APIs. But at least #523 is showing us improvements for some applications, and with a impl From<Body> for Response bound we could move this even further ahead (in a future patch):

// old API
res.body_json(Cat { name: "chashu" });

// new API
res.set_body(Body::from_json(Cat { name: "chashu" });

// http-rs/tide#523 (return type from function)
Ok(json!({ "name": "chashu" }));

// future option #1 
res.set_body(json!({ "name": "chashu" });

// future option #2 (return type from function)
Ok(Body::from_json(Cat { name: "chashu" }));

Either way, I hope this shows what the intended workflow around Response bodies is with this patch, and shows some options we could explore in the future to improve this further.

@yoshuawuyts yoshuawuyts merged commit 4276d67 into master Jun 1, 2020
@yoshuawuyts yoshuawuyts deleted the normalize-response branch June 1, 2020 12:51
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

Successfully merging this pull request may close these issues.

2 participants