Proxies can be sent as a string #300

paulefoe · 2018-06-04T10:05:47Z

this commit fixes #290

KostyaEsmukov

Thank you, I think this change would make API much better and more pleasant to use.

Just a couple of changes and it should be ready to go. As for the doc – I suggest you to not care about it for now. I wanted to rewrite the proxy section anyway, and this would be another good reason to finally get to it.

PS. Points for a good test! 😀

KostyaEsmukov · 2018-06-05T15:12:01Z

geopy/geocoders/base.py

@@ -185,6 +187,10 @@ def __init__(
                            else options.default_ssl_context)

        if self.proxies:
+            if isinstance(self.proxies, str):
+                self.scheme = self.proxies.split('://')[0]


First of all, proxies shouldn't affect self.scheme in any way.

Proxy scheme and URL scheme are totally different and doesn't need to match.

For example, for proxies = 'http://my_proxy_server:8080' and self.scheme = 'https' the client would send a CONNECT method to the proxy (in plaintext), and then start TLS negotiation with the remote using that TCP connection.

Just remove this line and it will be fine.

KostyaEsmukov · 2018-06-05T15:15:49Z

geopy/geocoders/base.py

@@ -185,6 +187,10 @@ def __init__(
                            else options.default_ssl_context)

        if self.proxies:
+            if isinstance(self.proxies, str):
+                self.scheme = self.proxies.split('://')[0]
+                self.proxies = {self.scheme: self.proxies}


Given that we verify above that self.scheme is either http or https, I would suggest to specify proxy for both schemes instead of the current one.

Consider this situation:

g = SomeGeocoder(proxies='http://my_proxy_server:8080', scheme='https') g.scheme = 'http'

After assignment proxy wouldn't be used, which is unexpected.

I guess I wasn't clear enough on this one.

This is what I meant:

self.proxies = {'http': self.proxies, 'https': self.proxies}

That way my snippet above will work correctly.

KostyaEsmukov · 2018-06-05T15:16:48Z

geopy/geocoders/base.py

@@ -185,6 +187,10 @@ def __init__(
                            else options.default_ssl_context)

        if self.proxies:
+            if isinstance(self.proxies, str):


For compatibility with py2, geopy.compat.string_compare should be used instead of str.

KostyaEsmukov · 2018-06-05T15:19:58Z

geopy/geocoders/base.py

@@ -70,6 +70,8 @@ class options(object):
            If specified, tunnel requests through the specified proxy.
            E.g., ``{"https": "192.0.2.0"}``. For more information, see
            documentation on :class:`urllib.request.ProxyHandler`.
+            Can be sent as a string ``"https://192.0.2.0"``, this will
+            automatically set the scheme to the ``https``


To be honest, I don't like it 🤔. But I'm not ready to propose a better phrasing for now as well.

Let's just keep it, but I'll get to this later after merging.

…e proxy

paulefoe · 2018-06-06T20:06:51Z

Thank you, I think this change would make API much better and more pleasant to use.

Thanks as usual for review and friendliness. Yeah, it's definitely going to be easier to use.

Just a couple of changes and it should be ready to go. As for the doc – I suggest you to not care about it for now. I wanted to rewrite the proxy section anyway, and this would be another good reason to finally get to it.

Thank you very much, putting the right thought into the docs is still the most tough part for me.

PS. Points for a good test! grinning

Thanks, I didn't do much just copied the exsiting test.

It took me some time to understand that there is a default scheme. This is why I put 2 tests, one with the https which is default, and one with http. But they both set explicitly, I think we can omit setting it explicitly in the https test, should we?

KostyaEsmukov · 2018-06-07T09:16:27Z

test/test_proxy.py

        base_http = urlopen(self.remote_website_http, timeout=self.timeout)
        base_html = base_http.read()
-        geocoder_dummy = DummyGeocoder(proxies=self.proxy_url,
+        geocoder_dummy = DummyGeocoder(proxies=self.proxy_url, scheme='http',


No, I don't think we need to test two schemes in two almost identical integration tests, which aren't cheap with regards to time.

It would be better to keep the previous single test, and add a new unit test here. Something like this:

g = DummyGeocoder(proxies=self.proxy_url, scheme='http') self.assertDictEqual(g.proxies, {'http': self.proxy_url, 'https': self.proxy_url})

That should be enough. We already tested that the g.proxies dict is respected in the integration tests, but we didn't check if both schemes are respected – which is exactly what this test does.

I am not sure if I get this right, but this will assert error:

self.assertDictEqual(g.proxies, {'http': self.proxy_url, 'https': self.proxy_url})

So I added 2 DummyGeocoders to check if both schemes are respected.

Have you by chance missed this comment? #300 (comment)

Also it would've been better if it was in a separate test_* function.

Oh yeah, I did miss this comment, pardon my inattention.

KostyaEsmukov

Thanks! Just a couple of simple corrections and it's ready to go 👍

KostyaEsmukov · 2018-06-12T15:51:58Z

test/test_proxy.py

+    def test_geocoder_constructor_uses_str_proxy(self):
+        base_http = urlopen(self.remote_website_http, timeout=self.timeout)
+        base_html = base_http.read()
+        geocoder_dummy = DummyGeocoder(proxies=self.proxy_url, scheme='http',


Is there any reason to set the scheme kwarg explicitly? If not, could you please remove that?

KostyaEsmukov · 2018-06-12T15:53:08Z

test/test_proxy.py

+        base_html = base_http.read()
+        geocoder_dummy = DummyGeocoder(proxies=self.proxy_url, scheme='http',
+                                       timeout=self.timeout)
+        print(geocoder_dummy.scheme, geocoder_dummy.proxies, 'proxy')


whoops 😅

KostyaEsmukov · 2018-06-13T17:01:16Z

Thanks! 🎉

paulefoe · 2018-06-14T14:20:11Z

Yay! Thank you very much, I'm looking forward to continue my work with you (can we call it this way?) 🎂

KostyaEsmukov · 2018-06-14T15:00:22Z

Of course, you're very welcome! I'm very grateful to you for your efforts on making geopy better. ✨🦄

proxies can be sent as a string now

392ed80

paulefoe changed the title ~~Proxies can be sent as a str~~ Proxies can be sent as a string Jun 4, 2018

KostyaEsmukov requested changes Jun 5, 2018

View reviewed changes

paulefoe added 4 commits June 6, 2018 15:13

changed self.proxies

5b4582a

added test for http and https, removed modification of a sche,e insid…

ca18e9b

…e proxy

fix

06f60fe

chanded str to string_compare for compatibility

0b180e5

KostyaEsmukov reviewed Jun 7, 2018

View reviewed changes

KostyaEsmukov added this to the 1.15 milestone Jun 7, 2018

KostyaEsmukov mentioned this pull request Jun 7, 2018

Proxies: rewrite doc, add examples with auth #301

Closed

paulefoe added 2 commits June 8, 2018 15:53

test for checking if both schemes are respected

bd18569

separete test for checking both schemes, changed proxies

e63a625

KostyaEsmukov reviewed Jun 12, 2018

View reviewed changes

removed print and unnecessary explicit scheme

7da916c

KostyaEsmukov approved these changes Jun 13, 2018

View reviewed changes

KostyaEsmukov merged commit e67a5bf into geopy:master Jun 13, 2018

paulefoe deleted the proxies branch June 14, 2018 14:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proxies can be sent as a string #300

Proxies can be sent as a string #300

Proxies can be sent as a string #300

Proxies can be sent as a string #300

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment