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: rustup #404

Merged
merged 1 commit into from
Mar 30, 2015
Merged

fix: rustup #404

merged 1 commit into from
Mar 30, 2015

Conversation

fhartwig
Copy link
Contributor

  • enable slice_patterns feature
  • add Reflect trait bounds where necessary

@GitCop
Copy link

GitCop commented Mar 29, 2015

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

  • Commit: 6a80399
    • 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

@fhartwig
Copy link
Contributor Author

Amended commit message to make gitcop happy.

@seanmonstar
Copy link
Member

Thanks! I wonder, if slice_patterns aren't going to be stable for beta/1.0,
we should probably not use them. I think they're only used in the
authorization header, and shouldn't be too hard to switch it to a few if
clauses.

On Sun, Mar 29, 2015, 9:18 AM Florian Hartwig notifications@github.com
wrote:

Amended commit message to make gitcop happy.


Reply to this email directly or view it on GitHub
#404 (comment).

@fhartwig
Copy link
Contributor Author

rust-lang/rust#23121 says that they hope to have it ready for the beta release, but I'm not sure if that still applies. I can re-write the function to avoid slice patterns if you like.

@seanmonstar
Copy link
Member

With beta this week, those comments don't assure me. If you could remove
the patterns, awesome. Or I can a little later.

On Sun, Mar 29, 2015, 9:57 AM Florian Hartwig notifications@github.com
wrote:

rust-lang/rust#23121 rust-lang/rust#23121
says that they hope to have it ready for the beta release, but I'm not sure
if that still applies. I can re-write the function to avoid slice patterns
if you like.


Reply to this email directly or view it on GitHub
#404 (comment).

* remove slice pattern
* add `Reflect` trait bounds where necessary
@fhartwig
Copy link
Contributor Author

Ok, slice patterns are gone. Tests pass for me, travis seems to have trouble with github today.

@@ -1,6 +1,7 @@
use std::fmt;
use std::str::{FromStr, from_utf8};
use std::ops::{Deref, DerefMut};
use std::marker::Reflect;
Copy link
Contributor

Choose a reason for hiding this comment

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

Reflect shouldn't be needed in this file as far as I can tell. What error comes up if you remove these bounds?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, since our Header traits should require Reflect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

error: the trait `core::marker::Reflect` is not implemented for the type `S` [E0277]

(in lines 26 and 47)

@reem
Copy link
Contributor

reem commented Mar 29, 2015

The Header downcasting stuff all needs Reflect bounds similar to Any and NetworkStream to be safe.

@Ryman
Copy link
Contributor

Ryman commented Mar 29, 2015

According to this the bound should be Any rather than Reflect + 'static as Any implies that?

@reem
Copy link
Contributor

reem commented Mar 29, 2015

@Ryman you're right. The existing Any bound should be sufficient.

seanmonstar added a commit that referenced this pull request Mar 30, 2015
@seanmonstar seanmonstar merged commit ce52315 into hyperium:master Mar 30, 2015
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