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

feat(header): add prefer and preference applied headers #750

Merged
merged 1 commit into from
Apr 1, 2016

Conversation

calebmer
Copy link
Contributor

Closes #747

The specification can be found here.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 90.487% when pulling 5fbc252 on calebmer:feat/prefer into 028f586 on hyperium:master.

@seanmonstar
Copy link
Member

Oh wow, this is extensive. Thanks for being thorough!

I was wondering in the issue #747 whether many people use this, because I was wondering if it should be part of hyper, or if the implementation can live in your own crate.

@calebmer
Copy link
Contributor Author

After a quick Google search, I found Microsoft Azure uses it (here is there specific extensions). Of course, as I mentioned in #747 the popular (~6,500 stars) PostgREST REST API uses it.

And a few other links of people using it that my Google search turned up:

@calebmer
Copy link
Contributor Author

On another note, I'm not sure why the test is failing, could you shed some light on that?

@seanmonstar
Copy link
Member

@calebmer seems the error was an intermittent. I restarted the build. I've also asked on IRC for some others to put feedback, as I have no experience with this header :)

fn from_str(s: &str) -> Result<Preference, Option<<u32 as FromStr>::Err>> {
use self::Preference::*;
let mut params = s.split(';').map(|p| {
let mut param: Vec<_> = p.splitn(2, '=').collect();
Copy link
Member

Choose a reason for hiding this comment

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

Just use the split iterator instead of allocating this temporary vector. It'd change the match to match (params.next(), params.next()).

Copy link
Member

Choose a reason for hiding this comment

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

Oh, and bump the indent for this closure please!

@seanmonstar
Copy link
Member

@calebmer I'll merge this after comments are addressed. Thanks again for such a thorough job.

@calebmer
Copy link
Contributor Author

Sorry about the indentation, my editor is set to two spaces and looks for an .editorconfig for other levels of indentation.

Comments addressed 😊👍

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 90.521% when pulling 6f64930 on calebmer:feat/prefer into c85b056 on hyperium:master.

@seanmonstar seanmonstar merged commit bc3878d into hyperium:master Apr 1, 2016
@seanmonstar
Copy link
Member

Excellent! Thank you!

@calebmer calebmer deleted the feat/prefer branch April 1, 2016 20:05
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.

3 participants