Skip to content

fix: rustup #404

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants