Add and use a Port struct for Uri::port() #252

joshleeb · 2018-09-14T23:26:37Z

Add the uri::Port struct an use it with Authority::port_path, and Uri::port_path methods, which deprecate Authority::port and Uri::port methods.

Closes #173 - Consider a Port type for Uri::port()

carllerche

Looks good. Left thoughts inline

carllerche · 2018-09-16T16:43:55Z

src/uri/authority.rs

-    /// Get the port of this `Authority`.
+    /// Get the port of this `Authority`. Deprecated: see `port_path` instead.
+    #[deprecated(since="0.2.0", note="please use `port_path` instead")]
+    pub fn port(&self) -> Option<u16> {


Could this be doc hidden as well?

carllerche · 2018-09-16T16:46:35Z

src/uri/authority.rs

@@ -196,28 +203,28 @@ impl Authority {
    ///
    /// # Examples
    ///
-    /// Authority with port
+    /// Authority with port path


I would keep the docs the same. No need to add “part”.

carllerche · 2018-09-16T16:48:30Z

src/uri/authority.rs

-        s.rfind(":").and_then(|i| {
-            u16::from_str(&s[i+1..]).ok()
-        })
+    pub fn port_path(&self) -> Option<Port> {


We have been using a _part suffix for this situation. We probably want to stick with that pattern.

carllerche · 2018-09-16T16:49:10Z

src/uri/mod.rs

-    /// Get the port of this `Uri`.
+    /// Get the port of this `Uri`. Deprecated: see `port_path` instead.
+    #[deprecated(since="0.2.0", note="please use `port_path` instead")]
+    pub fn port(&self) -> Option<u16> {


Same re doc hidden.

carllerche · 2018-09-16T16:51:23Z

src/uri/port.rs

+    /// assert!(Port::from_str("80").is_ok());
+    /// assert!(Port::from_str("abcd").is_err());
+    /// ```
+    pub fn from_str(bytes: &'a str) -> Result<Self, num::ParseIntError> {


I would not expose this fn yet in favor of not letting the type be publicly constructed yet.

joshleeb · 2018-09-17T13:44:33Z

@carllerche updated :)

carllerche

Thanks. It looks like there are a bunch of _pathreferences left. I pointed out a few, but there are some more.

This is probably causing CI to fail too.

carllerche · 2018-09-17T18:30:27Z

src/uri/authority.rs

@@ -180,12 +180,19 @@ impl Authority {
        host(self.as_str())
    }

-    /// Get the port of this `Authority`.
+    #[deprecated(since="0.2.0", note="please use `port_path` instead")]


Stray _path reference.

carllerche · 2018-09-17T18:30:55Z

src/uri/authority.rs

-    /// colon (":") character. A value is only returned if one is specified in
-    /// the URI string, i.e., default port values are **not** returned.
+
+    /// The port path subcomponent of authority is designated by an optional port


Stray "path" reference.

carllerche · 2018-09-17T18:31:31Z

src/uri/authority.rs

    /// let authority: Authority = "example.org:80".parse().unwrap();
    ///
-    /// assert_eq!(authority.port(), Some(80));
+    /// assert_eq!(authority.port_path(), Port::from_str("80").ok());


_path reference

carllerche · 2018-09-17T18:32:02Z

src/uri/authority.rs

    /// let authority: Authority = "example.org".parse().unwrap();
    ///
-    /// assert!(authority.port().is_none());
+    /// assert!(authority.port_path().is_none());


_path reference.

joshleeb · 2018-09-17T22:43:23Z

@carllerche should be fixed up properly now

bluetech · 2018-09-18T08:57:28Z

src/uri/port.rs

+    ///
+    /// The supplied `str` must be a valid u16.
+    pub(crate) fn from_str(bytes: &'a str) -> Result<Self, num::ParseIntError> {
+        u16::from_str(bytes).and_then(|port| Ok(Port { port, bytes }))


The BNF of a URI port is *DIGIT, see https://tools.ietf.org/html/rfc3986#section-3.2.3. However from_str allows an optional + sign, see https://doc.rust-lang.org/nightly/std/primitive.u16.html#method.from_str_radix.

In theory, this should already have been validated when constructing the Uri, so checking again here is not necessary.

carllerche

Looks good to me. I added some nits inline, but they are not required for the PR to land. @seanmonstar any follow up thoughts?

carllerche · 2018-09-18T15:21:40Z

src/uri/authority.rs

-    /// number in decimal following the host and delimited from it by a single
-    /// colon (":") character. A value is only returned if one is specified in
-    /// the URI string, i.e., default port values are **not** returned.
+    /// number as bytes following the host and delimited from it by a single


nit: I would remove as bytes here as that is an implementation detail.

carllerche · 2018-09-18T15:22:10Z

src/uri/authority.rs

-    /// the URI string, i.e., default port values are **not** returned.
+    /// number as bytes following the host and delimited from it by a single
+    /// colon (":") character. It can be turned into a decimal port number with
+    /// the `as_u16` method.


Maybe also add "or as a str with the as_str method.

carllerche · 2018-09-18T15:28:17Z

src/uri/mod.rs

-    /// number in decimal following the host and delimited from it by a single
-    /// colon (":") character. A value is only returned if one is specified in
-    /// the URI string, i.e., default port values are **not** returned.
+    /// number as bytes following the host and delimited from it by a single


Same doc nits as above.

carllerche · 2018-09-18T15:29:25Z

src/uri/port.rs

+    }
+}
+
+impl<'a> PartialEq for Port<'a> {


Not required for this PR, but it would be nice to add PartialEq implementations for numeric types (u16, u32, ...).

I'll put up a subsequent PR adding a generic implementation for PartialEq 😄

seanmonstar · 2018-09-18T18:32:09Z

Looks good to me when nits are fixed. Thanks!

carllerche requested changes Sep 16, 2018

View reviewed changes

carllerche requested changes Sep 17, 2018

View reviewed changes

Add and use a Port struct for Uri::port()

016377b

bluetech reviewed Sep 18, 2018

View reviewed changes

carllerche approved these changes Sep 18, 2018

View reviewed changes

Update docs for port_part uri and authority methods

12c1ff3

carllerche merged commit 0072b9d into hyperium:master Sep 20, 2018

joshleeb mentioned this pull request Sep 20, 2018

Add trait impls for uri::Port #255

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add and use a Port struct for Uri::port() #252

Add and use a Port struct for Uri::port() #252

joshleeb commented Sep 14, 2018

carllerche left a comment

carllerche Sep 16, 2018

carllerche Sep 16, 2018

carllerche Sep 16, 2018

carllerche Sep 16, 2018

carllerche Sep 16, 2018

joshleeb commented Sep 17, 2018

carllerche left a comment

carllerche Sep 17, 2018

carllerche Sep 17, 2018

carllerche Sep 17, 2018

carllerche Sep 17, 2018

joshleeb commented Sep 17, 2018 •

edited

bluetech Sep 18, 2018

carllerche Sep 18, 2018

carllerche left a comment

carllerche Sep 18, 2018

carllerche Sep 18, 2018

carllerche Sep 18, 2018

carllerche Sep 18, 2018

joshleeb Sep 19, 2018

seanmonstar commented Sep 18, 2018

Add and use a Port struct for Uri::port() #252

Add and use a Port struct for Uri::port() #252

Conversation

joshleeb commented Sep 14, 2018

carllerche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshleeb commented Sep 17, 2018

carllerche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshleeb commented Sep 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carllerche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanmonstar commented Sep 18, 2018

joshleeb commented Sep 17, 2018 •

edited