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

add proxy support, make httpClient instance variable [#15] #16

Merged
merged 1 commit into from
Aug 23, 2017

Conversation

dnl-blkv
Copy link
Contributor

No description provided.

@dnl-blkv dnl-blkv added this to the v0.10.0 milestone Aug 23, 2017
@dnl-blkv dnl-blkv requested a review from OGKevin August 23, 2017 15:48
@dnl-blkv
Copy link
Contributor Author

@OGKevin Had to make HttpClient an instance variable for a case if they want to use two different contexts with two different proxies within one process :)

Copy link
Contributor

@OGKevin OGKevin left a comment

Choose a reason for hiding this comment

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

Create issue for TODO please ☺️


private HttpClientHandler CreateHttpClientHandler()
{
// TODO: Add HTTP Public Key Pinning. It is needed to prevent possible man-in-the-middle attacks using
Copy link
Contributor

@OGKevin OGKevin Aug 23, 2017

Choose a reason for hiding this comment

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

Is this TODO supposed to be here ? If so could you remove it and create an issue for it with a link to this particular file and line ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This TODO is supposed to be there, and we would like to keep it there as TODO. Issue can be created additionally (you can do it if you wish :P).


public bool IsBypassed(Uri host)
{
return false; /* Proxy all requests */
Copy link
Contributor

Choose a reason for hiding this comment

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

Method doc 🤔 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, because this refers to the documentation. false means "Nothing is bypassed" which means "proxy all requests".

public BunqProxy(string proxyUri)
: this(new Uri(proxyUri))
{
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there really a need for 2 lines for this 😝 can this be reformatted ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@OGKevin this is in accordance with C# coding standard

@OGKevin OGKevin merged commit 33b5a78 into develop Aug 23, 2017
@OGKevin OGKevin deleted the 15-proxy branch August 23, 2017 17:57
@OGKevin
Copy link
Contributor

OGKevin commented Aug 23, 2017

@andrederoos

@OGKevin OGKevin mentioned this pull request Aug 23, 2017
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