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

Add context::Builder #24

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add context::Builder #24

wants to merge 6 commits into from

Conversation

Meralis40
Copy link
Contributor

Add context::Builder, a helper struct for creating a Context.
Also modify Router tests to use the new Builder.

Closes #22

}

/// Set an header
pub fn set_header<H: Header>(mut self, value: H) -> Self {
Copy link
Owner

Choose a reason for hiding this comment

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

Methods on a builder should be just header and not set_header. See a similar method on http::request::Builder: https://docs.rs/http/0.1.0/http/request/struct.Builder.html#method.header

}

/// Set the request data
pub fn set_data<B: Into<Body>>(mut self, body: B) -> Self {
Copy link
Owner

Choose a reason for hiding this comment

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

Same comment as set_header.

}

/// Create the `Context`, returning any error that occurs during build.
pub fn finalize(self) -> Result<Request, error::UriError> {
Copy link
Owner

Choose a reason for hiding this comment

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

This should probably be Result<Request, shio::Error> so we don't expose the hyper error type.

@mehcode
Copy link
Owner

mehcode commented Sep 20, 2017

Awesome job again and appreciate the fixes @Meralis40. Really busy this week so not sure when I'll have time to wrap this up.

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.

2 participants