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

don't use TLS when being redirected from http to https #1265

Merged
merged 1 commit into from
Apr 5, 2016

Conversation

MartinNowak
Copy link
Contributor

  • disconnect when changing host, schema, or port
  • add defaultPort to URL to simplify code

@@ -244,6 +244,7 @@ final class HTTPClient {
HTTPClientSettings m_settings;
string m_server;
ushort m_port;
bool use_tls;
Copy link
Member

Choose a reason for hiding this comment

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

Should be named m_useTLS.

@s-ludwig
Copy link
Member

Apart from the naming and the merge conflict in client.d, LGTM!

@s-ludwig
Copy link
Member

I was wondering if it would make sense to make the .port property of URL smart, so that it automatically uses the default port when no explicit port is given (and sets the port to 0 if it equals defaultPort). Maybe with an additional rawPort property that accesses the actual port written in the URL string representation.

- disconnect when changing host, schema, or port
- add defaultPort to URL to simplify code
@MartinNowak
Copy link
Contributor Author

Updated

I was wondering if it would make sense to make the .port property of URL smart, so that it automatically uses the default port when no explicit port is given

I think url.port ? url.port : url.defaultPort is short enough and less complex.

@MartinNowak
Copy link
Contributor Author

Ping @s-ludwig.

@s-ludwig
Copy link
Member

s-ludwig commented Apr 5, 2016

LGTM!

@s-ludwig s-ludwig merged commit 1ef9096 into vibe-d:master Apr 5, 2016
@MartinNowak MartinNowak deleted the fixDownload branch April 5, 2016 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants