-
Notifications
You must be signed in to change notification settings - Fork 314
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
Add trait impls for uri::Port #255
Conversation
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.
Thanks for following up. Some thoughts inline.
src/uri/port.rs
Outdated
impl<'a> fmt::Display for Port<'a> { | ||
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { | ||
write!(fmt, "{}", self.bytes) | ||
impl<'a, T: From<u16> + PartialEq> PartialEq<T> for Port<'a> { |
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 would rather avoid a blanket impl for this partialeq. I believe you did this to avoid having to add impls for each u* type. Perhaps a macro would work instead?
The problem is that adding this blanket impl would prevent us from adding a different blanket impl. Perhaps we would like to impl PartialEq<T>
for T: AsRef<Port>
or something.
src/uri/port.rs
Outdated
} | ||
} | ||
|
||
impl<'a> Into<&'a str> for Port<'a> { |
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.
It also looks like we could impl AsRef<str>
for Port
.
@carllerche updated 😄 |
src/uri/port.rs
Outdated
port_partialeq!(u32); | ||
port_partialeq!(u64); | ||
|
||
port_partialeq!(i16); |
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.
At a glance, an unchecked cast from u16
-> i16
is problematic unless there is a guarantee that the value will not be greater than i16::MAX
. Where is this checked?
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.
Added the check in
src/uri/port.rs
Outdated
} | ||
} | ||
|
||
impl<'a> Into<u16> for Port<'a> { |
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.
It's preferred to use the inverse of this, impl From<Port> for u16
. Otherwise, u16::from(port)
cannot work.
src/uri/port.rs
Outdated
impl<'a> fmt::Display for Port<'a> { | ||
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { | ||
write!(fmt, "{}", self.bytes) | ||
macro_rules! port_partialeq { |
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 personally would suggest just being for u16
, since that's what ports usually are. (Though, it'd be nice to remember the to implement PartialEq<Port> for u16
as well, to allow both port == num
and num == port
.)
Thanks! |
Implement some useful trait impls for
uri::Port
. This follows on from #252 and specifically this comment.