feat: Add TryFrom implementations for MetadataValue #990
Conversation
adamchalmers
force-pushed
the
more-metadata-conversions
branch
from
ff63a12
to
55fa803
Compare
|
As requested, tagging @LucioFranco for a review. |
adamchalmers
changed the title
| @@ -9,7 +9,7 @@ use tonic::{metadata::MetadataValue, transport::Channel, Request}; | |||
| async fn main() -> Result<(), Box<dyn std::error::Error>> { | |||
| let channel = Channel::from_static("http://[::1]:50051").connect().await?; | |||
|
|
|||
| let token = MetadataValue::from_str("Bearer some-auth-token")?; | |||
| let token: MetadataValue<_> = "Bearer some-auth-token".parse()?; | |||
There was a problem hiding this comment.
hmm I actually kinda prefer the look of from_str vs using parse. I wonder if we should not deprecate it and keep it. But just have the TryFrom's use it or the other way around?
There was a problem hiding this comment.
Hmm. So, MetadataValue impls FromStr, so it will already have a from_str method from stdlib, which is equivalent to parse. Unfortunately it's not in the stdlib prelude, so if I delete the ad-hoc from_str method then old code will fail to compile (although the compiler will suggest the easy fix of "use std::str::FromStr").
It does kinda annoy me to have an adhoc method named from_str when the stdlib one exists too, just because it seems to make the API a bit larger than it needs to be. But this is a very minor gripe :) So overall I'm happy with the solution of not deprecating from_str if that's what you'd like.
(I'm pretty used to parse() because it's used very often in the stdlib IP types).
So yeah, happy to do whichever, just let me know what you think now that you've ready this silly long reply :)
| /// let val = AsciiMetadataValue::try_from_bytes(b"\n"); | ||
| /// assert!(val.is_err()); | ||
| /// ``` | ||
| impl<'a, VE: ValueEncoding> TryFrom<&'a [u8]> for MetadataValue<VE> { |
There was a problem hiding this comment.
Can this lifetime be elided?
There was a problem hiding this comment.
It can be, but then Rustdoc generates docs with a signature like TryFrom<&'0 str>. If I name the lifetime, then it outputs the signature TryFrom<&'a str> which I think is more readable, more what Rust programmers are used to seeing.
I copied this style of doing it from the hyper docs -- happy to change it to the elided lifetime if you prefer, but I thought matching Hyper's docs is a good tiebreaker when I wasn't sure which was better.
There was a problem hiding this comment.
Oh cool didn't know that, seems fine to me!
LucioFranco
changed the title
|
Thanks! |
Motivation
As described in the issue there are several adhoc methods that say "This function is intended to be replaced in the future by a
TryFromimplementation once the trait is stabilized in std." Well, TryFrom is stable in std now, so let's use it :)Solution
I looked at which TryFrom impls http::HeaderValue has, and copied those where it made sense. Seems like MetadataValue is mostly a copy of http::HeaderValue, so I thought it was a good guide.
I tried to avoid repeating code, so often various TryFrom implementations either call into each other, or into the underlying
http::HeaderValueTryFrom implementations. I kept the #[inline] attribute when the original call had it, and added a deprecation warning to the original adhoc methods.Apparently MetadataValue already implements
Fromfor all numeric types >= 16 bits. They just don't show up in rustdoc, which is surprising, but I didn't dive in to figure out why. So I only implemented TryFrom around different string and byte representations.