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.

Sign up for GitHub

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

Jump to bottom

Revise Setup Tunnel Function #1389

Merged
merged 2 commits into from Jan 31, 2015
Merged

Revise Setup Tunnel Function #1389

merged 2 commits into from Jan 31, 2015

Conversation

seanstrom
Copy link
Contributor

Revise the setupTunnel function in request.js
Review? @nylen @simov @FredKSchott

@seanstrom seanstrom force-pushed the support/setup-tunnel-refactor branch from fe5be68 to 0c59203 Compare January 30, 2015 05:39
nylen
nylen reviewed Jan 30, 2015
View reviewed changes
request.js
var proxyHost = constructProxyHost(self.uri)
self.proxyHeaders = constructProxyHeaderWhiteList(self.headers, proxyHeaderWhiteList)
var proxyHeaders = constructProxyHeaderWhiteList(self.headers, proxyHeaderWhiteList)
self.proxyHeaders = proxyHeaders
Copy link
Member

Choose a reason for hiding this comment

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

why split this into 2 lines?

@nylen
Copy link
Member

nylen commented Jan 30, 2015

Looks good to me (doesn't change behavior), I'm not totally clear on all the edge cases for this code though (i.e. is it possible to override headers in the default lists, are the default header lists and custom lists all well-tested)

@seanstrom
Copy link
Contributor Author

Yeah Im totally sure this is all tested, Im going to look through the tests and see where this is tested.
I believe the files should be either test-tunnel.js or test-proxy.js

Do you know?

Update: above I mean't to say I am NOT totally sure aha.

@nylen
Copy link
Member

nylen commented Jan 30, 2015

test-proxy-connect.js, don't worry about it for this PR unless you see any issues. I say this a lot, more tests are always a good thing :)

@seanstrom
Copy link
Contributor Author

I accidentally forgot to say "NOT" when I said I was totally sure about something.
I want to add more tests, I was looking into the test coverage a bit and I am suspicious about the actual test coverage. It may not come into play this PR, but I am plan on doing an audit of sorts on Test Coverage.

And yes I totally agree with your opinion @nylen :)

nylen added a commit that referenced this pull request Jan 31, 2015
@nylen
Merge pull request #1389 from request/support/setup-tunnel-refactor
001c13f 
Revise Setup Tunnel Function
@nylen nylen merged commit 001c13f into master Jan 31, 2015
@seanstrom seanstrom deleted the support/setup-tunnel-refactor branch January 31, 2015 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@seanstrom @nylen