Mouse Position plugin by btozer · Pull Request #916 · python-visualization/folium

btozer · 2018-07-19T23:20:23Z

Original plugin from: Ardhi Lukianto
https://github.com/ardhi/Leaflet.MousePosition
MIT Licence
Copyright 2012 Ardhi Lukianto

stickler-ci · 2018-07-19T23:21:47Z

    'HeatMapWithTime',
    'MarkerCluster',
    'MeasureControl',
+     'MousePosition',


E131 continuation line unaligned for hanging indent

indent position corrected

Conengmo · 2018-07-21T07:47:28Z

+
+    https://github.com/ardhi/Leaflet.MousePosition
+
+    With the MIT License as below:


Good that you include the license. But could you put in in the file header? The docstring is converted to documentation and I don't want that littered with license texts.

Conengmo · 2018-07-21T07:48:05Z

+        {% endmacro %}
+        """)  # noqa
+
+    def __init__(self, position='bottomright', separator=' : ', emptyString='Unavailable', lngFirst=False, numDigits=5,


Please make this line 80 chars wide.

Conengmo · 2018-07-21T07:50:54Z

+
+    Parameters
+    ----------
+    position: location of the widget


Docstring convention is name, type and default value on the first line, description on the second. So something like:

position : str, default 'bottomright' Location of the widget.

Conengmo · 2018-07-21T07:52:08Z

+
+    def __init__(self, position='bottomright', separator=' : ', emptyString='Unavailable', lngFirst=False, numDigits=5,
+                 lngFormatter=None, latFormatter=None, prefix=""):
+        """Coordinate, linear, and area measure control"""


No need for a docstring on a class init when you already have the docstring above.

Conengmo · 2018-07-21T08:07:35Z

Thanks for this PR, it's a useful plugin! Check out my review comments for some details. Here are some longer comments:

Choose between specifying all arguments or using kwargs

Current convention in folium is to use Python style variable names throughout. That would mean that for example the arguments emptyString and lngFirst should become empty_string and lng_first up until they're added to the options dictionary.
Any variable that's an explicit argument should also be listed in the docstring parameters. So apart from position the other arguments should also be listed.

The fact you only included position in the docstring says something about the usefulness of the other arguments. You could decide to leave the other arguments out, and instead accept **kwargs and add those to the options. Then users can check out the original github project to see what those arguments are. Saves work, but is a bit lazy :) What do you want to do?

Add an example

Our current way of feature discovery is through an example gallery in Jupyter Notebooks. Please add an entry which shows how to use this plugin and its effect.

Optionally add a test

Not a strict necessity, but it would be nice if you can add a test for this module. You can check out the other plugin tests to see how they verify if the template is rendering correctly.

Changelog

Finally, just before merging, you should add a line in the changelog. Here's an example line from another plugin that was added:

- Added `FeatureGroupSubGroup` plugin (shtrom #875)

Parameters doc string reformatted. Licence shifted to header. __init__ < 80 chars.

stickler-ci · 2018-07-23T06:48:22Z

+
+from jinja2 import Template
+
+class MousePosition(MacroElement):


E302 expected 2 blank lines, found 1

stickler-ci · 2018-07-23T06:48:22Z

+    ----------
+    position : str, default 'bottomright'
+        Location of the widget.
+


W293 blank line contains whitespace

stickler-ci · 2018-07-23T06:48:23Z

+        {% endmacro %}
+        """)  # noqa
+
+    def __init__(self, position='bottomright', separator=' : ', 


W291 trailing whitespace

stickler-ci · 2018-07-23T06:48:23Z

+        """)  # noqa
+
+    def __init__(self, position='bottomright', separator=' : ', 
+                 emptyString='Unavailable', lngFirst=False, 


W291 trailing whitespace

stickler-ci · 2018-07-23T06:48:23Z

+
+    def __init__(self, position='bottomright', separator=' : ', 
+                 emptyString='Unavailable', lngFirst=False, 
+                 numDigits=5, lngFormatter=None, latFormatter=None, 


W291 trailing whitespace

stickler-ci · 2018-07-23T06:48:23Z

+                 emptyString='Unavailable', lngFirst=False, 
+                 numDigits=5, lngFormatter=None, latFormatter=None, 
+                 prefix=""):
+


W293 blank line contains whitespace

stickler-ci · 2018-07-23T06:54:33Z

+    ----------
+    position : str, default 'bottomright'
+        Location of the widget.
+


W293 blank line contains whitespace

stickler-ci · 2018-07-23T06:54:33Z

+                 emptyString='Unavailable', lngFirst=False,
+                 numDigits=5, lngFormatter=None, latFormatter=None,
+                 prefix=""):
+


W293 blank line contains whitespace

stickler-ci · 2018-08-21T22:09:51Z

+                 empty_string='Unavailable', lng_first=False,
+                 num_digits=5, lng_formatter=None, lat_formatter=None,
+                 prefix=""):
+


W293 blank line contains whitespace

btozer · 2018-08-21T23:54:28Z

Finally got around to this:

Choose between specifying all arguments or using kwargs

Current convention in folium is to use Python style variable names throughout. That would mean that for example the arguments emptyString and lngFirst should become empty_string and lng_first up until they're added to the options dictionary.
-> Done

Any variable that's an explicit argument should also be listed in the docstring parameters. So apart from position the other arguments should also be listed.
-> Done

The fact you only included position in the docstring says something about the usefulness of the other arguments. You could decide to leave the other arguments out, and instead accept **kwargs and add those to the options. Then users can check out the original github project to see what those arguments are. Saves work, but is a bit lazy :) What do you want to do?
-> Added all the parameters to the docstring.

Add an example

Our current way of feature discovery is through an example gallery in Jupyter Notebooks. Please add an entry which shows how to use this plugin and its effect.
-> Done see examples/plugin-MousePosition.ipynb

Optionally add a test

Not a strict necessity, but it would be nice if you can add a test for this module. You can check out the other plugin tests to see how they verify if the template is rendering correctly.
-> Skipped this

Changelog

Finally, just before merging, you should add a line in the changelog. Here's an example line from another plugin that was added:
-> Done - Added MousePosition plugin (btozer #916)

Conengmo · 2018-09-02T20:07:20Z

I made two changes:

The formatter option didn't work, because the functions were passed as string, where they should be Javascript functions. So instead I load them directly into the template. Note that in a Jupyter notebook this still gives an error, but I'm not really interested in solving that :) Instead I added an example in the docstring.
In the example notebook I put the second example in a new map, because the two MousePosition objects were interfering. I noticed that because the lng_first option was not working.

Maybe you have anything to add to this? Otherwise I'll go merge this in a few days.

ocefpaf · 2018-09-02T20:07:26Z

@@ -0,0 +1,120 @@
+# -*- coding: utf-8 -*-
+
+# CONVERTED TO PYTHON FROM: https://github.com/ardhi/Leaflet.MousePosition


You are using the plugin and not really converting it to Python.
I'm not a lawyer but I don't think this is necessary.

ocefpaf · 2018-09-02T20:07:51Z

+                                            'if it is not in a Figure.')
+
+        figure.header.add_child(
+            JavascriptLink('https://cdn.rawgit.com/ardhi/Leaflet.MousePosition/c32f1c84/src/L.Control.MousePosition.js'))  # noqa


We are already using noqa so no need for the extra line here.

ocefpaf · 2018-09-02T20:10:41Z

Maybe you have anything to add to this? Otherwise I'll go merge this in a few days.

I have just one monor comment about the license header added and I'm +1 to merge this.

ocefpaf · 2018-09-02T20:25:51Z

+    """Add a field that shows the coordinates of the mouse position.
+
+    Uses the Leaflet plugin by Ardhi Lukianto under MIT license.
+    https://github.com/ardhi/Leaflet.MousePosition


Conengmo · 2018-09-03T20:23:28Z

Thanks @btozer!

btozer added 2 commits July 19, 2018 16:12

Update __init__.py

52ecb8f

Create mouse_position.py

8bc6a6c

stickler-ci reviewed Jul 19, 2018

View reviewed changes

Update __init__.py

1153205

indent position corrected

Conengmo reviewed Jul 21, 2018

View reviewed changes

Conengmo mentioned this pull request Jul 21, 2018

Release v0.6.0 #915

Closed

Update mouse_position.py

00081d8

Parameters doc string reformatted. Licence shifted to header. __init__ < 80 chars.

stickler-ci reviewed Jul 23, 2018

View reviewed changes

btozer added 2 commits July 22, 2018 23:50

Update mouse_position.py

bccd13a

Update mouse_position.py

83e9e1a

stickler-ci reviewed Jul 23, 2018

View reviewed changes

updated parameter descriptions

f426da6

stickler-ci reviewed Aug 21, 2018

View reviewed changes

btozer added 5 commits August 21, 2018 15:26

added MousePosition example

f243652

updated mouseposition plugin

55bc861

updated mouse_control plugin

7b42227

updated change log

53d1fed

updated change log

426cfba

btozer and others added 5 commits August 21, 2018 16:59

updated mouseposition example

465fa3a

updated mouseposition example

20340a2

Cosmetic changes

57eecb0

Fix lat/lon formatter option

8499466

Fix example notebook

441ae11

ocefpaf reviewed Sep 2, 2018

View reviewed changes

Move attribution to docstring

5c27d31

Remove newline in links

2c55f49

ocefpaf reviewed Sep 2, 2018

View reviewed changes

Conengmo mentioned this pull request Sep 2, 2018

MarkerCluster: icon_create_function is not inserted as function #945

Closed

Conengmo added 6 commits September 2, 2018 23:07

Simpler adding function to options

26ff0d8

If formatter is None set to undefined

603cfde

Add formatter example to notebook

65a997f

Cosmetic changes

4660151

Changelog

a74796b

Merge branch 'master' into master

a71c282

Conengmo merged commit 1ea034a into python-visualization:master Sep 3, 2018


		https://github.com/ardhi/Leaflet.MousePosition

		With the MIT License as below:


		from jinja2 import Template

		class MousePosition(MacroElement):

		@@ -0,0 +1,120 @@
		# -- coding: utf-8 --

		# CONVERTED TO PYTHON FROM: https://github.com/ardhi/Leaflet.MousePosition

Conversation

btozer commented Jul 19, 2018

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!

Conengmo commented Jul 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

btozer commented Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Conengmo commented Sep 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ocefpaf commented Sep 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Conengmo commented Sep 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conengmo commented Jul 21, 2018 •

edited

Loading

btozer commented Aug 21, 2018 •

edited

Loading