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

TcpStream of CloneTcpStream should be public #530

Closed
wants to merge 2 commits into from

Conversation

apetrelli
Copy link

For the sake of extension, I made TcpStream of CloneTcpStream public.

@GitCop
Copy link

GitCop commented May 15, 2015

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

  • Commit: 03262d3
    • Commits must be in the following format: %{type}(%{scope}): %{description}
  • Commit: 38d44fc
    • 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

@apetrelli
Copy link
Author

Resolves #529

@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.42% when pulling 38d44fc on apetrelli:master into 7179651 on hyperium:master.

@seanmonstar
Copy link
Member

Is there a place where you could access it? I thought I made into_inner()
skip the wrapper.

On Fri, May 15, 2015, 7:50 AM Antonio Petrelli [email protected]
wrote:

Resolves #529 #529


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

@apetrelli
Copy link
Author

I'm trying to extend HttpConnector to support proxies, so I created an attempt of my version of HttpConnector, named HttpProxyConnector, copying code locally. The offending line is this:
Ok(HttpStream::Http(CloneTcpStream(try!(TcpStream::connect(addr)))))

Notice that I am a novice in Rust, so please pardon my lack of precision.

@seanmonstar
Copy link
Member

Ahhh, I see. Hm, I find CloneTcpStream a wart that I'd rather remove.

Question though: would proxy support in the Client be useful, or you're using the connectors directly?

@apetrelli
Copy link
Author

Probably I missed proxy support in client :-D I did not find it in docs, is it there somewhere? If not then probably I should open an issue in the docs :-D

@seanmonstar
Copy link
Member

No, there isn't proxy support in Client, yet. However, it should. And that'd be the best way, since it doesn't really require another connector.

@seanmonstar
Copy link
Member

@apetrelli ive opened #531 where we can discuss exactly how it should work.

I'm going to close this for now, as I'd rather find a way to remove CloneTcpStream, and making that property public may mean more code breaks when I finally do give it the axe.

@apetrelli
Copy link
Author

Ok fine for me.

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.

4 participants