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

No_proxy support #1096

Merged
merged 7 commits into from
Oct 1, 2014
Merged

No_proxy support #1096

merged 7 commits into from
Oct 1, 2014

Conversation

samcday
Copy link
Contributor

@samcday samcday commented Sep 25, 2014

Should resolve #620 and #853

As per a comment in the code, I used this page as inspiration for how no_proxy should work: http://lynx.isc.org/current/breakout/lynx_help/keystrokes/environments.html

@samcday
Copy link
Contributor Author

samcday commented Sep 29, 2014

/cc @nylen - in case you didn't see this one :)

@nylen
Copy link
Member

nylen commented Sep 29, 2014

Looks good, but I do see one minor issue. Here's the condition for matching hostname:

hostname.indexOf(noProxyHost) === hostname.length - noProxyHost.length

This means that requests to google.com will not be proxied if no_proxy contains oogle.com. I'm sure there are other ways, but you could fix this by setting:

var hostname = self.uri.hostname.replace(/^\.*/, '.');
// ...
var noProxyHost = noProxyItemParts[0].replace(/^\.*/, '.');

Then, we could use a couple of tests for the case I mentioned above and maybe 1-2 more where the no_proxy variable is set but the proxy should still be used.

Finally, a couple of minor style nits:

  • Move var hostname out of the for loop
  • Use if ( rather than if(, and for ( rather than for(

With the above changes, I'd merge this.

@samcday
Copy link
Contributor Author

samcday commented Sep 29, 2014

Thanks for the feedback. I've made the changes.

Good spot on the fuzzy matching of hostname. That would have been a very unfortunate and hard to diagnose bug :O

I also realised that the no_proxy hostnames were case sensitive, so I've fixed that (and added a test).

FYI I tried to follow style as closely as I could. Turns out I was cargo culting if( because that's the style in the code that immediately precedes the new stuff I added. I tried my best! :)

Anyway, I think I've followed up all your comments. Let me know if you have any more, or if I missed anything.

@nylen
Copy link
Member

nylen commented Sep 29, 2014

Ah ok, the if ( spacing is just one of those minor OCD things that makes me twitchy.

The only issue I see now is that you have 3 test files which incorrectly say "proxy should NOT be called" in the comments.

I don't love that these tests are in separate files, but IMO this is our fault because our tests don't use a sane framework (see #1003). So, I'm inclined to accept this as-is and merge these tests into 1-2 files when we rewrite them.

If you fix the "should NOT be called" comments, I'll wait until Wednesday to give others a chance to weigh in, and merge it if no one else has by then.

@samcday
Copy link
Contributor Author

samcday commented Sep 29, 2014

Bah, right you are. After the tests expanded from what I expected to be 2-3 simple tests into what it is now, I got a bit lazy with the copy/pasta ;)

Should be all good now. I'll check in on Wednesday and see if this is good for merge :)

@nylen
Copy link
Member

nylen commented Oct 1, 2014

Thanks @samcday!

nylen added a commit that referenced this pull request Oct 1, 2014
@nylen nylen merged commit 20b7989 into request:master Oct 1, 2014
nylen added a commit to nylen/request that referenced this pull request Oct 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need proxy_exclude option
2 participants