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

fix(server): Removed check for GET/HEAD request when parsing body #699

Merged
merged 2 commits into from
Nov 30, 2015
Merged

fix(server): Removed check for GET/HEAD request when parsing body #699

merged 2 commits into from
Nov 30, 2015

Conversation

kaedroho
Copy link
Contributor

Fixes #698

@GitCop
Copy link

GitCop commented Nov 29, 2015

Thanks for contributing! Unfortunately, I'm here to tell you there were the following style issues with your Pull Request:

  • Commit: d6f1980
    • Commits must be in the following format: %{type}(%{scope}): %{description}
  • Commit: 650c466
    • Commits must be in the following format: %{type}(%{scope}): %{description}

Guidelines are available at https://github.com/hyperium/hyper/blob/master/CONTRIBUTING.md


This message was auto-generated by https://gitcop.com

@kaedroho kaedroho changed the title Removed check for GET/HEAD request when parsing body fix(server): Removed check for GET/HEAD request when parsing body Nov 29, 2015
@@ -9,7 +9,7 @@ use std::time::Duration;
use buffer::BufReader;
use net::NetworkStream;
use version::{HttpVersion};
use method::Method::{self, Get, Head};
use method::Method::{self};

Choose a reason for hiding this comment

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

So this last '::{self}' part can be removed now.

Hmm in this diff it seems that this use statement can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed + squashed into original commit. Thanks!

@bombless
Copy link

I like this PR as we should do things right.

");

// FIXME: Use Type ascription
let mock: &mut NetworkStream = &mut mock;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should be able to write &mut mock as &mut NetworkStream here (which you could also just inline into the following line).

@reem
Copy link
Contributor

reem commented Nov 29, 2015

Giving the spec another close read, it seems that both the existing and proposed behaviors are legal, since the spec doesn't tie any defined semantics to GET or HEAD requests with bodies. Since reading the body is more flexible and I've seen GET requests with bodies in the wild, I think we should do this.

seanmonstar added a commit that referenced this pull request Nov 30, 2015
fix(server): Removed check for GET/HEAD request when parsing body
@seanmonstar seanmonstar merged commit d1f4a8b into hyperium:master Nov 30, 2015
@seanmonstar
Copy link
Member

Indeed, it seems this is better behavior. Thanks for the patch!

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.

5 participants