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(client): remove config requirement for Connect #1199

Merged
merged 1 commit into from
Jun 3, 2017
Merged

fix(client): remove config requirement for Connect #1199

merged 1 commit into from
Jun 3, 2017

Conversation

daboross
Copy link
Contributor

@daboross daboross commented Jun 2, 2017

Remove requirement when calling client::Config::connector() that the connector implements Connect.

Removing this requirement allows users to set the connector back to UseDefaultConnector. Previously,
this was not possible.

I'm not sure if there's any specific reason for requiring Connect on this method, as far as I can tell from past commits it was just an included part in the original design?

My use case for this is setting the connector back to UseDefaultConnector, as well as opening up the possibility for a UseHttpsConnector or a similar marker type to be used in a wrapper builder around Config.

Tests fail, but as far as I can tell this is only because of deprecated things that hyper is using. Locally, if #![deny(warnings)] is temporarily commented out, all tests pass.

Remove requirement when calling client::Config::connector() that the connector implements Connect.

Removing this requirement allows users to set the connector back to UseDefaultConnector. Previously,
this was not possible.
@seanmonstar
Copy link
Member

Hm, I suppose it's just there to help you catch if you pass something that isn't a Connect, but with this change, you'll still see such an error when you go to build(), so this seems fine!

Let me look into these build errors...

@daboross
Copy link
Contributor Author

daboross commented Jun 3, 2017

Thanks!

The build errors just seem to be mostly some renames that futures did, would you mind if I just submitted another PR changing those in hyper?

@seanmonstar seanmonstar merged commit 76fc633 into hyperium:master Jun 3, 2017
@seanmonstar
Copy link
Member

Fantastic, thanks for this and the deprecation fixes!

@daboross
Copy link
Contributor Author

daboross commented Jun 3, 2017

No problem, thank you!

@daboross daboross deleted the patch-1 branch June 3, 2017 22:01
daboross added a commit to daboross/rust-screeps-api that referenced this pull request Jun 3, 2017
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.

2 participants