Add value checking for location #625 #626

radumas · 2017-05-29T17:19:00Z

Using property decorators.
Need to implement testing.

Using property decorators.

stickler-ci · 2017-05-29T17:19:22Z

folium/map.py

@@ -297,6 +297,26 @@ def render(self, **kwargs):
            '</style>'), name='map_style')

        super(LegacyMap, self).render(**kwargs)
+


W293 blank line contains whitespace

stickler-ci · 2017-05-29T17:19:23Z

folium/map.py

+    @property
+    def location(self):
+        return self._location
+


W293 blank line contains whitespace

stickler-ci · 2017-05-29T17:19:23Z

folium/map.py

+        """Validates the location values before setting"""
+        if type(value) not in [list, tuple]:
+            raise TypeError("Location is not a list, expecting ex: location=[45.523, -122.675]")
+


W293 blank line contains whitespace

stickler-ci · 2017-05-29T17:19:23Z

folium/map.py

+
+        if len(value) != 2:
+            raise ValueError("Location should have two values, [lat, lon]")
+


W293 blank line contains whitespace

stickler-ci · 2017-05-29T17:19:24Z

folium/map.py

+
+        for val in value:
+            if not isinstance(val, numbers.Rational):
+                raise TypeError("Location values should be numeric, {val} is not a number".format(val))


E501 line too long (103 > 100 characters)

stickler-ci · 2017-05-29T17:19:24Z

folium/map.py

+        for val in value:
+            if not isinstance(val, numbers.Rational):
+                raise TypeError("Location values should be numeric, {val} is not a number".format(val))
+


W293 blank line contains whitespace

stickler-ci · 2017-05-29T17:19:24Z

folium/map.py

+            if not isinstance(val, numbers.Rational):
+                raise TypeError("Location values should be numeric, {val} is not a number".format(val))
+
+


W293 blank line contains whitespace

stickler-ci · 2017-05-29T17:19:24Z

folium/map.py

+                raise TypeError("Location values should be numeric, {val} is not a number".format(val))
+
+
+        self._location = value


E303 too many blank lines (2)

Bugfix on type checking numbers

stickler-ci · 2017-05-29T19:18:34Z

folium/map.py

+                float(val)
+            except:
+                raise TypeError("Location values should be numeric, {} is not a number".format(val))
+


W293 blank line contains whitespace

radumas · 2017-05-29T20:02:54Z

I'll wait to fix that whitespace error until there are other comments on this PR

ocefpaf · 2017-05-31T13:03:30Z

folium/map.py

+
+    @property
+    def location(self):
+        return self._location


I am not sure we need a property and a setter for this. Can you explain what you have in mind?

I thought that we could have an internal function to check and raise a ValueError that we would call in the places we need to check for the two value list.

That's a fair point. I was more worried about changing anything in an __init()__ function, since it seems like the property decorators leave those alone, but it does result in unnecessary code. I could swap out for a.... class function?

Not sure we need a class function. The location is used only once to create the map. We don't need to store it as a property b/c we won't do anything with it later. Just a private function to convert to float and issue a ValueError when that cannot be done would suffice IMO. Then you can run that function in __init__(), it would be just 1 extra line there. What do you think?

ocefpaf · 2017-05-31T13:04:39Z

folium/map.py

+            raise TypeError("Location is not a list, expecting ex: location=[45.523, -122.675]")
+
+        if len(value) != 2:
+            raise ValueError("Location should have two values, [lat, lon]")


I like that message. Simple and clear!

ocefpaf · 2017-05-31T13:08:32Z

folium/map.py

+            try:
+                float(val)
+            except:
+                raise TypeError("Location values should be numeric, {} is not a number".format(val))


In a way this is still a ValueError b/c some strings will pass your float(val) line. But that is not too important.

PS: I never tested if

location = ['12.5', 42.5]

would work on leaflet. But that will pass your test there.

oh that's an interesting point. I guess I could actually convert the values to float or number and then we wouldn't need to know whether it works in leaflet or not.

Yep. I would go for that approach.

ocefpaf · 2017-06-24T22:11:14Z

@radumas what is the status on this?

I am planning to isseu a new release and it would be nice to get this PR merged before that.

radumas · 2017-06-26T19:46:46Z

@ocefpaf I was waiting for your response to my replies to your comments.

ocefpaf · 2017-06-27T10:38:56Z

@ocefpaf I was waiting for your response to my replies to your comments.

Sorry. My time for folium is so limited that I always need to start from zero when I get back to PRs...

Remove property setters based on PR comments

stickler-ci · 2017-06-27T19:47:55Z

folium/map.py

@@ -60,6 +60,19 @@
     'https://rawgit.com/python-visualization/folium/master/folium/templates/leaflet.awesome.rotate.css'),  # noqa
    ]

+def _format_lat_lon(values):


E302 expected 2 blank lines, found 1

stickler-ci · 2017-06-27T19:47:55Z

folium/map.py

+            values = [float(val) for val in values]
+        except:
+            raise ValueError("Location values should be numeric, {} is not a number".format(val))
+        return value


F821 undefined name 'value'

radumas · 2017-06-27T19:50:36Z

Totally understandable. I've updated based on comments.

ocefpaf · 2017-06-27T20:01:50Z

folium/map.py

+        try:
+            values = [float(val) for val in values]
+        except:
+            raise ValueError("Location values should be numeric, {} is not a number".format(val))


ocefpaf · 2017-06-27T20:02:59Z

folium/map.py

@@ -60,6 +60,19 @@
     'https://rawgit.com/python-visualization/folium/master/folium/templates/leaflet.awesome.rotate.css'),  # noqa
    ]

+def _format_lat_lon(values):


How about naming it _validate_location?

stickler-ci · 2017-06-27T20:47:31Z

folium/map.py

@@ -60,6 +60,20 @@
     'https://rawgit.com/python-visualization/folium/master/folium/templates/leaflet.awesome.rotate.css'),  # noqa
    ]

+def _validate_location(values):


E302 expected 2 blank lines, found 1

Just this one left to address. I'll merge as is and fix it later. Thanks @radumas!

stickler-ci · 2017-06-27T20:47:31Z

folium/map.py

@@ -170,7 +184,7 @@ def __init__(self, location=None, width='100%', height='100%',
            self.location = [0, 0]
            self.zoom_start = min_zoom
        else:
-            self.location = location
+            self.location = _format_lat_lon(location)


F821 undefined name '_format_lat_lon'

Just as I pushed this I realized this mistake... sigh

Bugfix on type checking numbers

…t_check Add value checking for location python-visualization#625

Add value checking for location python-visualization#625

8abfe0a

Using property decorators.

stickler-ci reviewed May 29, 2017

View reviewed changes

folium/map.py Outdated

@property

def location(self):

return self._location

Copy link

stickler-ci May 29, 2017

Choose a reason for hiding this comment

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

W293 blank line contains whitespace

stickler-ci reviewed May 29, 2017

View reviewed changes

Continue python-visualization#626

eedbf9d

Bugfix on type checking numbers

stickler-ci reviewed May 29, 2017

View reviewed changes

ocefpaf reviewed May 31, 2017

View reviewed changes

Merge branch 'master' into location_init_check

d36233e

Update location check to function

2ac2628

Remove property setters based on PR comments

stickler-ci reviewed Jun 27, 2017

View reviewed changes

Typo fix

d17ccfd

ocefpaf reviewed Jun 27, 2017

View reviewed changes

Change function name

22f3435

stickler-ci reviewed Jun 27, 2017

View reviewed changes

Finish renaming function

2d278a5

ocefpaf merged commit cdca018 into python-visualization:master Jun 27, 2017

ocefpaf mentioned this pull request Aug 23, 2017

Value Check on Location parameter of the Map creator #625

Closed

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

Continue python-visualization#626

492504e

Bugfix on type checking numbers

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

Merge pull request python-visualization#626 from radumas/location_ini…

489c222

…t_check Add value checking for location python-visualization#625

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add value checking for location #625 #626

Add value checking for location #625 #626

radumas commented May 29, 2017

stickler-ci May 29, 2017

stickler-ci May 29, 2017

stickler-ci May 29, 2017

stickler-ci May 29, 2017

stickler-ci May 29, 2017

stickler-ci May 29, 2017

stickler-ci May 29, 2017

stickler-ci May 29, 2017

stickler-ci May 29, 2017

radumas commented May 29, 2017

ocefpaf May 31, 2017

radumas May 31, 2017

ocefpaf Jun 27, 2017

ocefpaf May 31, 2017

ocefpaf May 31, 2017

radumas May 31, 2017

ocefpaf Jun 27, 2017

ocefpaf commented Jun 24, 2017 •

edited

radumas commented Jun 26, 2017

ocefpaf commented Jun 27, 2017

stickler-ci Jun 27, 2017

stickler-ci Jun 27, 2017

radumas commented Jun 27, 2017

ocefpaf Jun 27, 2017

ocefpaf Jun 27, 2017

stickler-ci Jun 27, 2017

ocefpaf Jun 27, 2017

stickler-ci Jun 27, 2017

radumas Jun 27, 2017

		@@ -297,6 +297,26 @@ def render(self, **kwargs):
		'</style>'), name='map_style')

		super(LegacyMap, self).render(**kwargs)


		if len(value) != 2:
		raise ValueError("Location should have two values, [lat, lon]")

		if not isinstance(val, numbers.Rational):
		raise TypeError("Location values should be numeric, {val} is not a number".format(val))

		raise TypeError("Location values should be numeric, {val} is not a number".format(val))


		self._location = value

Add value checking for location #625 #626

Add value checking for location #625 #626

Conversation

radumas commented May 29, 2017

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

radumas commented May 29, 2017

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

ocefpaf commented Jun 24, 2017 • edited

radumas commented Jun 26, 2017

ocefpaf commented Jun 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radumas commented Jun 27, 2017

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

ocefpaf commented Jun 24, 2017 •

edited