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

Support for Windows system proxy settings #274

Merged
merged 16 commits into from May 9, 2017

Conversation

Projects
None yet
2 participants
@varosi
Contributor

varosi commented May 6, 2017

Related to #268

Code is copy pasted from HTTP module, have some additional fixes and integrated with http-client modules. I'm open to any suggestions for fixes.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg May 8, 2017

Owner

I'd prefer if the other code changes be in a separate PR. Some of them are things I would rather not have included, and in any event it makes it more difficult to see what changes are introduced for this feature.

In addition, usage of pure seems to have broken the build for GHC < 7.10.

Owner

snoyberg commented May 8, 2017

I'd prefer if the other code changes be in a separate PR. Some of them are things I would rather not have included, and in any event it makes it more difficult to see what changes are introduced for this feature.

In addition, usage of pure seems to have broken the build for GHC < 7.10.

@varosi

This comment has been minimized.

Show comment
Hide comment
@varosi

varosi May 8, 2017

Contributor

What changes do you mean by "other"? What you would like not to be included?

Contributor

varosi commented May 8, 2017

What changes do you mean by "other"? What you would like not to be included?

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg May 8, 2017

Owner

Anything not directly related to the change at hand, such as changing whitespace or reformatting imports. Some of the changes (like aligning records) is something I wouldn't include, as it makes diffs more noisy in the future.

Owner

snoyberg commented May 8, 2017

Anything not directly related to the change at hand, such as changing whitespace or reformatting imports. Some of the changes (like aligning records) is something I wouldn't include, as it makes diffs more noisy in the future.

@varosi

This comment has been minimized.

Show comment
Hide comment
@varosi

varosi May 8, 2017

Contributor

Okay, I'll revert reformatting, no problem.

Contributor

varosi commented May 8, 2017

Okay, I'll revert reformatting, no problem.

@varosi

This comment has been minimized.

Show comment
Hide comment
@varosi

varosi May 8, 2017

Contributor

Done. Something else?

Contributor

varosi commented May 8, 2017

Done. Something else?

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg May 8, 2017

Owner

Cursory review looks good, thanks. I'll review in more detail and hopefully merge tomorrow.

Owner

snoyberg commented May 8, 2017

Cursory review looks good, thanks. I'll review in more detail and hopefully merge tomorrow.

@varosi

This comment has been minimized.

Show comment
Hide comment
@varosi

varosi May 8, 2017

Contributor
Contributor

varosi commented May 8, 2017

-- Under Windows try first env. variables override then Windows proxy settings
#if defined(mingw32_HOST_OS)
modifier' <- systemProxy prot

This comment has been minimized.

@snoyberg

snoyberg May 9, 2017

Owner

It looks like it would be more efficient to check the env vars first and, if that is Nothing, then call systemProxy. Right now, if modifier is Just, we'll have an unnecessary systemProxy call.

@snoyberg

snoyberg May 9, 2017

Owner

It looks like it would be more efficient to check the env vars first and, if that is Nothing, then call systemProxy. Right now, if modifier is Just, we'll have an unnecessary systemProxy call.

This comment has been minimized.

@varosi

varosi May 10, 2017

Contributor

hm, it's not so simple. I have two actions:

envProxy' :: IO (HostAddress -> Maybe ProxySettings)
systemProxy' :: IO (HostAddress -> Maybe ProxySettings)

How to join the two with the second depending on the result of the inner function of the first?

@varosi

varosi May 10, 2017

Contributor

hm, it's not so simple. I have two actions:

envProxy' :: IO (HostAddress -> Maybe ProxySettings)
systemProxy' :: IO (HostAddress -> Maybe ProxySettings)

How to join the two with the second depending on the result of the inner function of the first?

@snoyberg snoyberg merged commit eba8bb8 into snoyberg:master May 9, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg May 9, 2017

Owner

Thanks!

Owner

snoyberg commented May 9, 2017

Thanks!

snoyberg added a commit that referenced this pull request May 10, 2017

@varosi varosi deleted the varosi:varosi/win_proxy branch May 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment