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

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

Closed
seanmonstar opened this issue Feb 20, 2018 · 8 comments
Closed

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

seanmonstar opened this issue Feb 20, 2018 · 8 comments

Comments

@seanmonstar
Copy link
Member

Users may wish to access the port as a string, and not just an integer. We have the string already, so we could provide a newtype around it, Port, and a user can then get the bytes themselves, or just ask for the integer.

fn port(&self) -> Option<Port>;
struct Port<'a> {
    bytes: &'a str,
}

impl<'a> Port<'a> {
    fn as_str(&self) -> &str;

    fn as_u16(&self) -> u16;
}

This would be for both Uri and Authority. Probably introduced as port_part(), deprecating port() to be replaced in 0.2.

@carllerche
Copy link
Collaborator

You'd probably want to add a bunch of PartialEq implementations as well.

@briansmith
Copy link

briansmith commented Feb 20, 2018

impl<'a> Port<'a> {
    fn as_str(&self) -> &str;

    fn as_u16(&self) -> u16;
}

These would be better as From/Into conversions.

@carllerche
Copy link
Collaborator

No reason to not have both!

@joshleeb
Copy link
Contributor

joshleeb commented Sep 9, 2018

@seanmonstar got a PR in progress for implementing Port as

struct Port<'a> {
    bytes: &'a str,
}

Is there any particular benefit to having bytes: &'a str rather than storing the port as a u16? Then we could have something like

struct Port {
    port: u16,
}

impl Port {
    fn from_bytes(bytes: &str) -> Option<Port>) {
        u16::from_str(bytes).and_then(|p| Some(p.into())).ok()
    }
    fn to_string(&self) -> String { self.port.to_string() }
    fn as_u16(&self) -> String { self.port } // Also impl From<u16> for Port {}
}

@carllerche
Copy link
Collaborator

The main reason would be to avoid parsing if not needed 🤷

@seanmonstar
Copy link
Member Author

@joshleeb people can already get the u16 directly, the purpose of this type is for cases where you need the port but in string form. With only the u16 accessor, you have to allocate a new string via port.to_string(), but we already have the &str inside the Authority.

@joshleeb
Copy link
Contributor

Do we want to pull out the valid port number, as a &str and store it in the Port struct? Something like

pub fn new(bytes: &'str) -> Option<Port<'a>> {
    bytes
        .rfind(":")
        .and_then(|i| u16::from_str(bytes).ok())
        .and_then(|_| Some(Port{ bytes }))
}

Or do we just want to store the raw bytes, even if it's an invalid port number?

@seanmonstar
Copy link
Member Author

Hm, yea, since the port isn't verified to be a valid u16 when parsing the full URI, it'd be weird to get a Port that couldn't be turned into a u16. Because of that, it does make sense to parse to a u16 first. Perhaps to not require parsing again, the Port could have a u16 field as well.

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

No branches or pull requests

4 participants