-
Notifications
You must be signed in to change notification settings - Fork 83
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
Mime: add From<&str> #179
Mime: add From<&str> #179
Conversation
@@ -139,6 +139,12 @@ impl FromStr for Mime { | |||
} | |||
} | |||
|
|||
impl<'a> From<&'a str> for Mime { | |||
fn from(value: &'a str) -> Self { | |||
Self::from_str(value).unwrap() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use expect
here instead? Probably copy over the message from FromStr
's error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I follow - FromStr
here doesn't have an error message itself, the message comes directly from parse, which would probably provide a more useful error than this would.
I suppose this may actually be better as TryFrom
, but that does mean updating Tide internals to support it, and also, what would it do with such an error anyways?
I'm actually having a bit of 2nd thoughts on this, given our panic/internal unwrap discussion the other day -- do we really want to encourage people to write e.g. res.set_content_type("application/json")
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really want to encourage people to write (...)
That's the idea; we already support this for status codes, headers, urls, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant, shouldn't we encourage people to use res.set_content_type(mime::TYPE)
, res.set_header(headers::HEADER, ...)
instead, since that is more guaranteed to not have a name parsing panic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That aside though, what would expect()
say that wouldn't already be more detailed from parse?
d73606c
to
ca2f461
Compare
ca2f461
to
68c1866
Compare
@yoshuawuyts I don't know how this can be improved aside from making it |
Refs: http-rs/tide#575