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

Jump to bottom

Add method_ref, uri_ref, headers_ref to request::Builder #284

Merged
merged 3 commits into from Dec 19, 2018
Merged

Add method_ref, uri_ref, headers_ref to request::Builder #284

merged 3 commits into from Dec 19, 2018

Conversation

kazuk
Copy link
Contributor

@kazuk kazuk commented Dec 13, 2018

implementation for #282

@kazuk kazuk mentioned this pull request Dec 13, 2018
Closed
carllerche
carllerche requested changes Dec 13, 2018
View reviewed changes
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.

Thanks for doing this. Lets see what @seanmonstar thinks about adding these APIs.

src/request.rs Outdated Show resolved Hide resolved
src/request.rs Outdated Show resolved Hide resolved
seanmonstar
seanmonstar requested changes Dec 13, 2018
View reviewed changes
Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

I forgot that the builder carries an error, and so these all need to return Options. That's a bummer.

I don't have much of an opinion on get_*, really. An alternative is also *_ref, like method_ref(). I don't know if that's any better...

src/request.rs Outdated Show resolved Hide resolved
src/request.rs Outdated Show resolved Hide resolved
src/request.rs Outdated Show resolved Hide resolved
@kazuk
apply review comments
71726ca 
change function name
 get_method -> method_ref
 get_uri -> uri_ref
 get_headers -> header_ref

avoid clone, and returns reference
@kazuk
Copy link
Contributor Author

kazuk commented Dec 14, 2018

thanks comments, I done to apply change request.

I found some diff on cargo fmt, Can I apply formatting to request.rs in this PR?

@kazuk kazuk changed the title Add get_method, get_uri, get_headers to request::Builder Add method_ref, uri_ref, headers_ref to request::Builder Dec 14, 2018
@carllerche
Copy link
Collaborator

@kazuk please avoid formatting unrelated parts of code.

@kazuk
Copy link
Contributor Author

kazuk commented Dec 19, 2018 •
edited

Something Can I do for this PR?

carllerche
carllerche approved these changes Dec 19, 2018
View reviewed changes
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. I'll let @seanmonstar do a final review + merge.

I like the _ref suffix pattern for this.

seanmonstar
seanmonstar approved these changes Dec 19, 2018
View reviewed changes
Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Thanks for sticking with it!

@seanmonstar seanmonstar merged commit 5018e48 into hyperium:master Dec 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carllerche carllerche carllerche approved these changes

@seanmonstar seanmonstar seanmonstar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kazuk @carllerche @seanmonstar