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

Allow using a proxy #168

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

tim95030
Copy link

@tim95030 tim95030 commented Aug 2, 2016

I added this in a way that nothing is affected unless you define a proxy with PGoApi.set_proxy or by passing in a proxy to the PGoApiRequest method. I defaulted it to None and it uses the standard requests library if it is none.

@tim95030 tim95030 force-pushed the add-proxy-support branch 2 times, most recently from ab75962 to 703f5d4 Compare August 2, 2016 18:19
@tejado
Copy link
Owner

tejado commented Aug 3, 2016

Can you please check why travis-ci is failing?

@tim95030
Copy link
Author

tim95030 commented Aug 3, 2016

I have been working on that. It appears to be an issue with Python3, but I don't know how to fix it yet. I haven't had time to figure this out at the moment. I should be able to this weekend.

@DSlayerMan
Copy link

The current requests library supports proxies and socks. http://docs.python-requests.org/en/master/user/advanced/#proxies

@tim95030
Copy link
Author

tim95030 commented Aug 3, 2016

Ok. I will fix it up to use requests[socks] then. Didn't realize they added that.

@beeblebox
Copy link

@tim95030 check your fb messages

@@ -90,7 +93,7 @@ def _make_rpc(self, endpoint, request_proto_plain):
request_proto_serialized = request_proto_plain.SerializeToString()
try:
http_response = self._session.post(endpoint, data=request_proto_serialized)
except requests.exceptions.ConnectionError as e:
except (requests.exceptions.ConnectionError, requesocks.exceptions.ConnectionError) as e:
Copy link

Choose a reason for hiding this comment

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

Mayby throw different exception for proxy error?

Then it could be catched in client and "new" proxy set..

Copy link
Author

@tim95030 tim95030 Aug 4, 2016

Choose a reason for hiding this comment

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

Fair enough. Also I need to remove the requesocks.exception here since I replaced that library in my latest change.

[Edit]: Upon further inspection I am not sure what I can do here. There is already a raise here that can be caught in an application. I did remove the exception from resquesocks since I removed that library.

@ndurchx
Copy link

ndurchx commented Aug 5, 2016

I think, the authentification should also use the proxy.

@tim95030
Copy link
Author

tim95030 commented Aug 5, 2016

@ndurchx What do you mean? All requests use the proxy with this change. You turn on the proxy and all requests use it. If you mean it should support proxies with authentication, then that was not my goal and can be added later separately.

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.

7 participants