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 support for Socks5 proxy. #518

Merged
merged 8 commits into from
Feb 25, 2017
Merged

Conversation

daimajia
Copy link
Contributor

@daimajia daimajia commented Feb 6, 2017

fix #517

I'm not quite sure if I'm doing the right thing. 🤣

Please comment if I'm making mistake. 🗣

@jh0ker
Copy link
Member

jh0ker commented Feb 13, 2017

It looks okay to me, did you do a manual test to see if it works for you?

@jh0ker
Copy link
Member

jh0ker commented Feb 13, 2017

BTW, you also have to edit the requirements accordingly (at least for travis) to prevent this error:

/home/travis/virtualenv/python2.7.9/lib/python2.7/site-packages/urllib3/contrib/socks.py:37: DependencyWarning: SOCKS support in urllib3 requires the installation of optional dependencies: specifically, PySocks.  For more information, see https://urllib3.readthedocs.io/en/latest/contrib.html#socks-proxies```

@daimajia
Copy link
Contributor Author

@jh0ker Get it. Thanks for your tips.

@daimajia
Copy link
Contributor Author

@jh0ker I did a manual test, and it works well.

@daimajia
Copy link
Contributor Author

@jh0ker Any idea on this issue?

@jh0ker
Copy link
Member

jh0ker commented Feb 13, 2017

@daimajia that is expected behavior, no need for your to worry

@daimajia
Copy link
Contributor Author

BTW. This issue

It looks all right for this PR. 😃

@jh0ker
Copy link
Member

jh0ker commented Feb 13, 2017

It would be preferred to list the dependency as an extra dependency, similar to ujson. You'd have to put it in setup.py. To prevent import errors on systems where it is not installed, I recommend you move the import into the if that starts in line 97 of request.py

@daimajia
Copy link
Contributor Author

Good suggestion. I'll finish it ASAP.

@daimajia
Copy link
Contributor Author

daimajia commented Feb 13, 2017

@jh0ker Remove the PySocks dependency from requirements.txt, right? I'm not sure.

@daimajia
Copy link
Contributor Author

@jh0ker How about now?

@jh0ker
Copy link
Member

jh0ker commented Feb 13, 2017

@daimajia yes, please

@daimajia
Copy link
Contributor Author

@jh0ker Done, please check it.

@tsnoam
Copy link
Member

tsnoam commented Feb 13, 2017

@daimajia Thank you for your good work. That's something we'd really like.

  1. I performed a very minimal test for your PR with ssh socks5 proxy and it works for me 👍
  2. There are two minor things I would like to fix before I merge (and already have the commits ready) but I can't push to your repo/branch (and save the ping pong of asking you to do them). Can you allow the maintainers to push? See: https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

@daimajia
Copy link
Contributor Author

daimajia commented Feb 13, 2017

image

right? @tsnoam

@tsnoam
Copy link
Member

tsnoam commented Feb 14, 2017

@daimajia
yes, it is good. must have been my fault earlier. anyway i pushed my small changes and after Travis will finish unitests, I believe we'll be able to merge.

@daimajia
Copy link
Contributor Author

@tsnoam Thanks. ❤️

@tsnoam
Copy link
Member

tsnoam commented Feb 25, 2017

@daimajia
sorry for the delay. the failure in CI is irrelevant to your changes, so I'm merging the PR.
Thanks for the good work.
BTW, I see that you haven't added yourself to AUTHORS.rst. If you'd like being mentioned there, feel free to send a second PR.

@tsnoam tsnoam merged commit e39afad into python-telegram-bot:master Feb 25, 2017
@daimajia
Copy link
Contributor Author

@tsnoam Thanks, I'm pretty willing to add myself to the contributors list, I'm proud of contributing to this lib. 😊

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support socks5 proxy.
3 participants