Skip to content

Allow header #188

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.

Sign up for GitHub

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
Merged
merged 1 commit into from
Dec 10, 2014
Merged

Allow header #188

merged 1 commit into from
Dec 10, 2014

Conversation

ajnirp
Copy link
Contributor

@ajnirp ajnirp commented Dec 10, 2014

@ajnirp
Copy link
Contributor Author

ajnirp commented Dec 10, 2014

I couldn't figure out how to get the empty Vec subtest to work. I was getting an assertion fail panic:

(left: `Some(Allow([]))`, right: `Some(Allow([]))`)`

@reem
Copy link
Contributor

reem commented Dec 10, 2014

Can you rebase this to get rid of the merge commit?

reem
reem reviewed Dec 10, 2014
View reviewed changes
src/header/common/allow.rs
mod tests {
use super::Allow;
use header::Header;
use super::super::super::super::method::Method::{mod, Options, Get, Put, Post, Delete, Head, Trace, Connect, Patch, Extension};
Copy link
Contributor

Choose a reason for hiding this comment

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

you should be able to just use method::Method::..

@ajnirp
Copy link
Contributor Author

ajnirp commented Dec 10, 2014

Rebased, also added a bench

seanmonstar
seanmonstar reviewed Dec 10, 2014
View reviewed changes
src/header/common/allow.rs
impl HeaderFormat for Allow {
fn fmt_header(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
let Allow(ref parts) = *self;
fmt_comma_delimited(fmt, parts[])
Copy link
Member

Choose a reason for hiding this comment

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

This can be just 'self[]' because of Deref.

@seanmonstar
Copy link
Member

What test was failing?

@ajnirp
Copy link
Contributor Author

ajnirp commented Dec 10, 2014

@seanmonstar I tried to test for an empty vec.

let empty_vec: Vec<Method> = vec![];
let allow = Header::parse_header([b"".to_vec()].as_slice());
assert_eq!(allow, Some(allow(empty_vec)));

which was giving the above-mentioned error. I couldn't figure out what was wrong with this, so I removed the test before pushing.

@seanmonstar
Copy link
Member

Could it be that it's parsing "" into Extension("".to_string())?

Can't wait rust-lang/rfcs#504

@ajnirp
Copy link
Contributor Author

ajnirp commented Dec 10, 2014

Just checked, that was indeed the problem. Should we include this test, then? (asserting against Extension("".to_string()))

@seanmonstar
Copy link
Member

We should have the test that the vec is empty. That it's parsing into an Extension sounds like a bug in http::read_method.

@seanmonstar
Copy link
Member

Sorry, wrong place. The bug is here: https://github.com/hyperium/hyper/blob/master/src/method.rs#L82

@ajnirp
Copy link
Contributor Author

ajnirp commented Dec 10, 2014

Patched that, and also addressed the fmt_comma_delimited nit.

seanmonstar added a commit that referenced this pull request Dec 10, 2014
@seanmonstar
Merge pull request #188 from wenderen/allow-header
7f93184 
Allow header

Closes #174
@seanmonstar seanmonstar merged commit 7f93184 into hyperium:master Dec 10, 2014
@seanmonstar
Copy link
Member

woo! thanks

@ajnirp ajnirp deleted the allow-header branch December 10, 2014 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ajnirp @reem @seanmonstar