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

Introduce a Uri Builder #219

Merged
merged 1 commit into from Oct 8, 2018
Merged

Introduce a Uri Builder #219

merged 1 commit into from Oct 8, 2018

Conversation

seanmonstar
Copy link
Member

This introduces a Builder type for manipulating and building up Uris. Instead of having mutators on Uri directly, which would need to return errors, instead this suggests that a user either use a Builder from the start, or to convert a Uri into via uri.into_builder().

An example looks like:

let uri = Uri::builder()
    .scheme("https")
    .authority("hyper.rs")
    .path_and_query("/")
    .build()
    .unwrap();

It currently only supports setting the 3 distinct parts, not any sub parts (like host or path), but I expect those could be added. This is however generic over HttpTryFrom, so you can easily build pieces with &str, or Bytes, etc.

It's only a start, but related to #206.

@bluetech
Copy link

bluetech commented Jun 25, 2018

Uri as described in the docs has 4 distinct forms:

request-target = origin-form
               / absolute-form
               / authority-form
               / asterisk-form

Maybe it makes sense to have 4 distinct builders, one for each form? (Though the asterisk form doesn't really need one).

src/uri/mod.rs Outdated
@@ -289,6 +296,11 @@ impl Uri {
parse_full(s)
}

/// dox
pub fn into_builder(self) -> Builder {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe leave this fn off for now?

Copy link
Collaborator

@carllerche carllerche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm. Just needs docs

@@ -22,6 +22,22 @@ pub trait HttpTryFrom<T>: Sized + Sealed {
fn try_from(t: T) -> Result<Self, Self::Error>;
}

pub(crate) trait HttpTryInto<T>: Sized {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this added for internal convenience or does it impact the public api somehow.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, it was internal convenience, allowing arg.http_try_into() instead of <T as HttpTryFrom<U>>::try_from(arg). Since the trait isn't public, and no one can import it, it exposes no public contract.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me.

@carllerche
Copy link
Collaborator

Yeah, just needs docs + it looks like CI is busted for some reason.

@seanmonstar
Copy link
Member Author

@carllerche took a while, but I finally cleaned this up.

@seanmonstar seanmonstar merged commit 97e1b96 into master Oct 8, 2018
@seanmonstar seanmonstar deleted the uri-builder branch October 8, 2018 20:54
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

Successfully merging this pull request may close these issues.

None yet

3 participants