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

use http_types::mime instead of the mime crate #536

Merged
merged 6 commits into from
May 28, 2020

Conversation

jbr
Copy link
Member

@jbr jbr commented May 24, 2020

Fix the use of the mime crate in the graphql example (brought up by #534), which was a mistake. This pr removes dependence on the mime crate entirely, in preference to http_types::Mime

This pr also allows set_mime to accept an Into, and calls through to the http_types::Response::set_content_type

@jbr jbr force-pushed the use-http-types-mime branch 2 times, most recently from 3507a3a to 8d13df2 Compare May 24, 2020 03:59
@@ -70,7 +71,7 @@ impl<State> Endpoint<State> for ServeDir {
res.set_body(body);

if let Some(content_type) = mime_guess::from_path(&file_path).first() {
res = res.set_mime(content_type);
res = res.set_mime(content_type.to_string().parse::<Mime>().unwrap());
Copy link
Member

Choose a reason for hiding this comment

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

This is not very nice, should http_types::HeaderValue have an into for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this code is temporary and will be replaced by the mime sniffing stuff in http_types

Copy link
Member Author

Choose a reason for hiding this comment

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

This is converting from a mime::Mime to a http_types::Mime, and I think the goal is for neither tide nor http_types to have a dep on the mime crate

Copy link
Member

Choose a reason for hiding this comment

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

http-types Body now has a from_file constructor that applies the inference. Could as maybe use that here?

The trickiest part I see is checking the error type so that we can set the right status codes, but that should be possible through downcasting.

That way we can get rid of the mime dep effective immediately!

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in 5000e1c

Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

We should probably ensure in http-types that we return the right status codes here — but that's worth filing an issue for. Otherwise this looks good!

@yoshuawuyts yoshuawuyts merged commit a376bc6 into http-rs:master May 28, 2020
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.

3 participants