Added ability to use remote geo_path by jreades · Pull Request #602 · python-visualization/folium

jreades · 2017-03-06T21:10:19Z

Adds a dependency (requests) but enables ability to use http/https/ftp for loading geo_paths.

I'm really rusty on unit tests (last did them in Java 1.2 or thereabouts) so I've not attempted to implement these. This is the first time I've done this, so please review carefully before accepting the PR.

stickler-ci · 2017-03-06T21:10:38Z

        # Create GeoJson object
        if geo_path:
-            geo_data = open(geo_path)
+            if geo_path.lower().startswith(('http','ftp','https')): 


E231 missing whitespace after ','
W291 trailing whitespace

stickler-ci · 2017-03-06T21:10:39Z

-            geo_data = open(geo_path)
+            if geo_path.lower().startswith(('http','ftp','https')): 
+                geo_data = requests.get(geo_path).json()
+            else: 


W291 trailing whitespace

QuLogic · 2017-03-07T00:21:49Z

        # Create GeoJson object
        if geo_path:
-            geo_data = open(geo_path)
+            if geo_path.lower().startswith(('http', 'ftp', 'https')):


Probably should include the colon, too; someone could have a file that starts with any of these sequences.

Maybe it is an overkill but how about something like:

def scheme_validator(url): parsed = urlparse(url) if parsed.scheme in ['http', 'ftp', 'https']: return True return False

to validade the scheme?

ocefpaf · 2017-03-07T12:23:07Z

Adds a dependency (requests) but enables ability to use http/https/ftp for loading geo_paths.

I am fine with the extra dependency. We could use a try/except to catch it when unavailable and issue a proper error message, but I don't think it is worth in this case.

I'm really rusty on unit tests (last did them in Java 1.2 or thereabouts) so I've not attempted to implement these. This is the first time I've done this, so please review carefully before accepting the PR.

The PR is fine. You can add a simple test that just tries to use this. See https://github.com/python-visualization/folium/blob/master/tests/test_folium.py#L282 for an example (but yours will probably be much simpler than that.)

Please add:

requests to the requirements.txt file
a changelog entry in the CHANGES.txt file
the test 😉

and this should be good to go.

jreades · 2017-03-07T13:11:16Z

Cool, I will try to get the test written in the next few days.

I will modify the startswith to check for a colon since that is both trivial and sensible. I originally wrote this using urlparse (it's why the check didn't already have a :) but ended up thinking it was overkill.

Will modify docs and reqs accordingly.

…swith on URL checking.

stickler-ci · 2017-03-09T11:13:35Z

        # Create GeoJson object
        if geo_path:
-            geo_data = open(geo_path)
+            if geo_path.lower().startswith(('http:', 'ftp':, 'https:')):


E231 missing whitespace after ':'
E999 SyntaxError: invalid syntax

stickler-ci · 2017-03-09T14:30:46Z

 from six import PY3
 import branca.element

+import requests


F401 'requests' imported but unused

stickler-ci · 2017-03-09T14:30:47Z

+        self.map = folium.Map(zoom_start=4)
+
+        # Adding remote GeoJSON as additional layer.
+        path = 'https://raw.githubusercontent.com/python-visualization/folium/master/examples/data/us-states.json'


E501 line too long (114 > 100 characters)

stickler-ci · 2017-03-09T14:35:57Z


+# For testing remote requests
+remote_url = '/'.join([
+	'https://raw.githubusercontent.com',


E101 indentation contains mixed spaces and tabs
W191 indentation contains tabs

stickler-ci · 2017-03-09T14:35:57Z

+# For testing remote requests
+remote_url = '/'.join([
+	'https://raw.githubusercontent.com',
+	'python-visualization/folium/master',


W191 indentation contains tabs

stickler-ci · 2017-03-09T14:35:57Z

+remote_url = '/'.join([
+	'https://raw.githubusercontent.com',
+	'python-visualization/folium/master',
+	'examples/data/us-states.json'])


W191 indentation contains tabs

jreades · 2017-03-09T14:40:35Z

Right, think I've managed to tidy everything up -- that stickler really is a stickler (but I can appreciate that). Have added a simple test that tries to remotely access one of the example JSON files packaged with Folium from GitHub and checks that the map extent matches its expectations.

jreades · 2017-03-09T14:41:19Z

Please do check that edits to CHANGES.txt and such are appropriate and reasonable.

ocefpaf · 2017-03-09T14:44:06Z

 - Added `smooth_factor `option to `GeoJSON`, `TopoJSON` and `Choropleth` (JamesGardiner #428)
 - `Map` object now accepts Leaflet global switches (sgvandijk #424)
 - Added weight option to CircleMarker (palewire #581)
+- Added requests support to enable http(s) and ftp for geo_path parameter


If you want feel free to add you github handle and the PR number here too.

ocefpaf · 2017-03-09T14:44:31Z

+remote_url = '/'.join([
+    'https://raw.githubusercontent.com',
+    'python-visualization/folium/master',
+    'examples/data/us-states.json'])


Good one 👍

ocefpaf · 2017-03-09T14:46:23Z

Right, think I've managed to tidy everything up

Thanks! Looks good to go.

that stickler really is a stickler (but I can appreciate that).

And it saves the devs time while reviewing PRs 😉

Have added a simple test that tries to remotely access one of the example JSON files packaged with Folium from GitHub and checks that the map extent matches its expectations.

That is perfect. Let's wait on Travis-CI and merge this.

jreades · 2017-03-09T14:48:29Z

Cool — I’ve added my info to the CHANGES.txt file so I think we’re good to go. Thank you all for the support/tips on getting this through to merged. It’s not something I’ve done before with Travis and all these other tools so it could have been… hairy. jon

…

On 9 Mar 2017, at 14:46, Filipe ***@***.***> wrote: Right, think I've managed to tidy everything up Thanks! Looks good to go. that stickler really is a stickler (but I can appreciate that). And it saves the devs time while reviewing PRs 😉 Have added a simple test that tries to remotely access one of the example JSON files packaged with Folium from GitHub and checks that the map extent matches its expectations. That is perfect. Let's wait on Travis-CI and merge this. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#602 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADEzZe7o9GCCR3m0CQKPI2NVSu4gYy6tks5rkBDCgaJpZM4MUsut>.

-- Jon Reades m: 07976.987.392 e: jon@reades.com <mailto:jon@reades.com> im: jereades

ocefpaf · 2017-03-09T15:15:50Z

Thanks for the awesome contribution @jreades!

Added ability to use remote geo_path

jreades added 2 commits March 6, 2017 21:05

Added ability to use remote URL for GeoJSON

0a7dafb

Ooops, forgot to remove debugging message

5ec18b1

stickler-ci reviewed Mar 6, 2017

View reviewed changes

Fixing stickler stuff

c5c9dc4

QuLogic reviewed Mar 7, 2017

View reviewed changes

ocefpaf mentioned this pull request Mar 7, 2017

Fix doc bug #601

Closed

Adding details to CHANGES and requirements. Tweaked pattern for start…

b37df58

…swith on URL checking.

stickler-ci reviewed Mar 9, 2017

View reviewed changes

jreades added 2 commits March 9, 2017 11:15

Grrrr, typo.

3a8b75f

Added test for remote JSON.

d6fc34a

stickler-ci reviewed Mar 9, 2017

View reviewed changes

Responding to stickler feedback

7d7c19a

stickler-ci reviewed Mar 9, 2017

View reviewed changes

Responding to stickler feedback

a151002

ocefpaf reviewed Mar 9, 2017

View reviewed changes

Added handle and PR number.

7b5ce5b

ocefpaf merged commit 6753324 into python-visualization:master Mar 9, 2017

QuLogic mentioned this pull request Mar 24, 2017

Remote GeoJSON URLs in Folium (https? http?) #584

Closed

sanga pushed a commit to sanga/folium that referenced this pull request Oct 24, 2017

Merge pull request python-visualization#602 from jreades/master

25ac4d2

Added ability to use remote geo_path

Conversation

jreades commented Mar 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ocefpaf commented Mar 7, 2017

Uh oh!

jreades commented Mar 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreades commented Mar 9, 2017

Uh oh!

jreades commented Mar 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ocefpaf commented Mar 9, 2017

Uh oh!

jreades commented Mar 9, 2017 via email

Uh oh!

ocefpaf commented Mar 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants