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 #2

Merged
merged 4 commits into from Oct 5, 2016
Merged

Add proxy support #2

merged 4 commits into from Oct 5, 2016

Conversation

jamiel
Copy link
Contributor

@jamiel jamiel commented Oct 5, 2016

Adds proxy support to Net/Soap and Net/Http

@@ -133,6 +133,24 @@ public function setContent($content)
return $this->set('content', "$content");
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

One leading space should be removed from this 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.

Fixed

Copy link
Member

@Bilge Bilge Oct 5, 2016

Choose a reason for hiding this comment

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

Unfortunately it appears the extra space has been added to additional lines instead of removed from this one. All affected lines in this file should now be made consistent with the style of the rest of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, should all line up now.

@coveralls
Copy link

coveralls commented Oct 5, 2016

Coverage Status

Coverage increased (+0.04%) to 98.372% when pulling 1a0a1c3 on jamiel:proxy_support into 4449b11 on ScriptFUSION:master.

@Bilge
Copy link
Member

Bilge commented Oct 5, 2016

This looks like an excellent PR. Thank you for respecting the style of the files and including tests. Will you be wanting a new tag immediately to make use of these features?

@@ -60,6 +60,11 @@ public function testReplaceHeaders()
self::assertCount(2, $options->getHeaders());
}

public function testProxy()
{
self::assertSame('https://foo.com:80', (new HttpOptions)->setProxy('https://foo.com:80')->getProxy());
Copy link
Member

@Bilge Bilge Oct 5, 2016

Choose a reason for hiding this comment

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

I encourage developers to avoid specifying constants more than once in a test case. This slightly reduces maintenance burden if, for example, strings need to be updated in future in order for tests to pass. For example,

self::assertSame($host = 'https://foo.com:80', (new HttpOptions)->setProxy($host)->getProxy()); 

You can see this pattern used throughout the other test cases in this file. Also applies to SoapOptionsTest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I agree - that's been updated.

@coveralls
Copy link

coveralls commented Oct 5, 2016

Coverage Status

Coverage increased (+0.04%) to 98.372% when pulling 6d49f95 on jamiel:proxy_support into 4449b11 on ScriptFUSION:master.

@Bilge Bilge merged commit db670fa into ScriptFUSION:master Oct 5, 2016
@Bilge
Copy link
Member

Bilge commented Oct 5, 2016

Thanks!

@coveralls
Copy link

coveralls commented Oct 5, 2016

Coverage Status

Coverage increased (+0.04%) to 98.372% when pulling 099df3c on jamiel:proxy_support into 4449b11 on ScriptFUSION:master.

@jamiel
Copy link
Contributor Author

jamiel commented Oct 5, 2016

No problem, a tag would definitely be appreciated!

@Bilge
Copy link
Member

Bilge commented Oct 6, 2016

This has been tagged as 🏷️ 0.7.2.

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.

None yet

3 participants