GeoJsonTooltip warn for GeometryCollection #988

jtbaker · 2018-10-14T20:34:36Z

Closes #929. Checks geometry types for GeometryCollections, stores the feature IDs of any in a list, and warns the user of incompatibility, and suggests refactoring to MultiPolygon geometry type.

Added a new Tooltip class to reference GeoJson feature properties, with tests, documentation, and examples. It's flexible, and can take field names, aliases, that can be turned on/off with the 'labels' boolean, a 'sticky' property, and can incorporate Javascript's .toLocaleString() functionality if desired. The same string can be passed for each object with the 'text' variable as well if you'd rather not use the 'fields'. Also updated the GeoJson JS template to reference this Tooltip object, and added a test to make sure that the values passed exist in the properties.

Further linting.

Seems like maybe the f-string usage in the geojson fields assertion check is causing the build failure. Testing normal string concatenation.

Reverted accidental vega_parse change.

Fixed docstring indentation.

Updating Tooltip docstring broke 120 char line limit.

Removed trailing white space.

Updating my fork to current Master.

…m/jtbaker/folium to local

…iLang.xml, ide.general.xml, Default.xml, project.default.xml, diff.xml, git.xml, other.xml, jdk.table.xml, Default.xml, code.style.schemes, find.xml, ui.lnf.xml, laf.xml, filetypes.xml, projectView.xml, vcs.xml, editor.codeinsight.xml, notifications.xml, keymap.xml, editor.xml, github_settings.xml, debugger.xml

…, IntelliLang.xml, ide.general.xml, Default.xml, project.default.xml, diff.xml, git.xml, other.xml, jdk.table.xml, Default.xml, code.style.schemes, find.xml, ui.lnf.xml, laf.xml, filetypes.xml, projectView.xml, vcs.xml, editor.codeinsight.xml, notifications.xml, keymap.xml, editor.xml, github_settings.xml, debugger.xml" This reverts commit 76c9622.

…ithub.com/jtbaker/folium to local" This reverts commit 84811a0.

Conflicts: vcs.xml

onto separate lines so it is more readable.

jtbaker · 2018-10-15T23:59:35Z

Also a suggestion: what do you think about putting this in a separate method? I like to split of distinct functionality in separate methods when it makes sense. For example this could become def warn_for_geometry_collection() or something, with its own docstring explaining why we're doing this.

Ok, that sounds fine to me. To clarify, do you envision it as a validation function that lives in utilities.py to pass GeoJsonTooltip's self._parent.data through in the render method? Or as a class method only?

Conengmo · 2018-10-16T05:08:27Z

I was thinking simply a class method. The code is only for GeoJsonTooltip, so moving it to some common place such as utilities is not needed.

Conengmo · 2018-10-16T05:19:30Z

folium/features.py

+                warnings.warn(
+                    "GeoJsonTooltip is not configured to render tooltips for GeoJson GeometryCollection geometries. "
+                    "Please consider reworking feature IDs {} to MultiPolygon for full functionality.\n"
+                    "https://tools.ietf.org/html/rfc7946#page-9""".format(geom_collections), UserWarning)


Note that there are still three "s here.

Whoops! Fixed.

Fix last string quote from triple to single.

Conengmo · 2018-10-24T10:22:13Z

There is one more thing I would like to do in this PR: add a test that verifies the warning is triggered when a geometry collection is used. That way we can be confident the validation function is right, also in the future. It's hard to see if a validation function is working properly without a test, because it's trigger might be rare.

Is that something you want to try @jtbaker? If not that's okay, I can pick it up or we'll leave it out.

…re collections will have an 'id' key.

stickler-ci · 2018-10-29T07:06:59Z

tests/test_features.py

@@ -104,3 +106,14 @@ def test_color_line():
        opacity=1)
    m.add_child(color_line)
    m._repr_html_()
+
+def test_geojson_tooltip():


E302 expected 2 blank lines, found 1

stickler-ci · 2018-10-29T07:07:00Z

tests/test_features.py

+
+def test_geojson_tooltip():
+    m = folium.Map([30.5, -97.5], zoom_start=10)
+    geojson = folium.GeoJson("./kuntarajat.geojson",


F841 local variable 'geojson' is assigned to but never used

stickler-ci · 2018-10-29T07:07:00Z

tests/test_features.py

+    m = folium.Map([30.5, -97.5], zoom_start=10)
+    geojson = folium.GeoJson("./kuntarajat.geojson",
+                             tooltip=folium.GeoJsonTooltip(
+                                 fields=['code','name']


E231 missing whitespace after ','

stickler-ci · 2018-10-29T07:07:00Z

tests/test_features.py

+    with warnings.catch_warnings(record=True) as w:
+        warnings.simplefilter('always')
+        m._repr_html_()
+        assert isinstance(w[-1].category, UserWarning), "GeoJsonTooltip GeometryCollection test failed."


W292 no newline at end of file

stickler-ci · 2018-10-29T07:10:01Z

tests/test_features.py

+def test_geojson_tooltip():
+    m = folium.Map([30.5, -97.5], zoom_start=10)
+    folium.GeoJson("./kuntarajat.geojson",
+                    tooltip=folium.GeoJsonTooltip(fields=['code','name'])


E127 continuation line over-indented for visual indent
E231 missing whitespace after ','

stickler-ci · 2018-10-29T07:11:40Z

tests/test_features.py

+def test_geojson_tooltip():
+    m = folium.Map([30.5, -97.5], zoom_start=10)
+    folium.GeoJson("./kuntarajat.geojson",
+                   tooltip=folium.GeoJsonTooltip(fields=['code','name'])


E231 missing whitespace after ','

Conengmo · 2018-10-30T07:00:29Z

Looks good @jtbaker! This is what I had in mind. Unfortunately the test is not working yet in the Travis build. Here is the error:

> assert isinstance(w[-1].category, UserWarning), "GeoJsonTooltip GeometryCollection test failed."
E AssertionError: GeoJsonTooltip GeometryCollection test failed.
E assert False
E +  where False = isinstance(<class 'UserWarning'>, UserWarning)
E +    where <class 'UserWarning'> = <warnings.WarningMessage object at 0x7f328400d7f0>.category

It seems the isinstance test is not working as intended. Looking at the Pytest docs they give two methods for checking warnings:

The first is to use a with pytest.warns(UserWarning): context for the call that's expected to generate a warning.
https://docs.pytest.org/en/latest/warnings.html#warns

The second is to check recorded warnings, like you did. But in their example the don't use isinstance but issubclass: assert issubclass(w.category, UserWarning)
https://docs.pytest.org/en/latest/warnings.html#recwarn

Can you try any of these two methods to get the test working?

Conengmo

This looks good Jason! Unfortunately the test is currently failing due to an unrelated problem. I already made a fix and will open a PR with it. Should be quick to merge, after than we can update this PR and merge this.

Conengmo · 2018-12-09T10:02:56Z

Merged! Thanks Jason. Merging upstream master into your branch went well I see.

jtbaker added 30 commits June 14, 2018 19:49

Cleaned up code linting for this commit.

2491e19

Update features.py

6290a5b

Further linting.

Further linting.

fda9729

Hopefully last linting.

bde7a2b

Update features.py

b5f9282

Update features.py

8e5168c

Update features.py

73b1f4d

lint

d03bf2f

lint

8ce80a6

lint

bb325f4

lint

bd2a12f

lint

c73686e

lint

5b9cb8b

Update features.py

5525df0

Update features.py

cc89971

Update features.py

bb825f0

More lint checking.

5c9c425

Seems like maybe the f-string usage in the geojson fields assertion check is causing the build failure. Testing normal string concatenation.

Update features.py

16d44f0

Update features.py

2fb3437

Reverted accidental vega_parse change.

Update features.py

9933f58

Fixed docstring indentation.

Update features.py

c8d404d

Updating Tooltip docstring broke 120 char line limit.

Update features.py

90d7de9

Removed trailing white space.

Merge pull request #1 from python-visualization/master

bfd277b

Updating my fork to current Master.

PC-2018.1.2 <jasonbaker@Earl.local Overwrite remote https://github.co…

84811a0

…m/jtbaker/folium to local

Revert "PC-2018.1.2 <jasonbaker@Earl.local Overwrite remote https://g…

cf116fe

…ithub.com/jtbaker/folium to local" This reverts commit 84811a0.

PC-2018.1.2 <jasonbaker@Earl.local Update vcs.xml

f9e3eb8

PC-2018.1.2 <jasonbaker@Earl.local Merge branch 'master'

d9f4109

Conflicts: vcs.xml

Rework Warning lines, and split geom_collection list comprehension

d909e4b

onto separate lines so it is more readable.

Conengmo reviewed Oct 16, 2018

View reviewed changes

Rework check to class method.

071b220

Fix last string quote from triple to single.

Conengmo changed the title ~~GeoJsonTooltip Update~~ GeoJsonTooltip warn for GeometryCollection Oct 24, 2018

Conengmo added the waiting for changes This PR has been reviewed and changes are needed before merging label Oct 28, 2018

jtbaker added 3 commits October 29, 2018 02:04

Add file with GeometryCollection geometry for test.

2bdfb99

Refactor warning for more general feature warning since not all featu…

2a09446

…re collections will have an 'id' key.

Add Test for GeometryCollection warning with GeoJsonTooltip.

7ecb4fe

stickler-ci reviewed Oct 29, 2018

View reviewed changes

Stickler fixes.

19f6d9b

stickler-ci reviewed Oct 29, 2018

View reviewed changes

jtbaker added 2 commits October 29, 2018 02:10

Stickler fixes.

4aebbc9

Stickler fixes.

4a8eec2

stickler-ci reviewed Oct 29, 2018

View reviewed changes

jtbaker added 2 commits October 29, 2018 02:12

Stickler fixes.

d913b41

Filepath name fix for CI integration.

bf68fa9

jtbaker and others added 2 commits November 19, 2018 22:53

Change test to subclass method. Sorry for the delay on this @Conengmo.

8de05c3

Merge branch 'master' into GeoJsonTooltipUpdate

cf71841

Conengmo reviewed Nov 25, 2018

View reviewed changes

Conengmo added blocked This issue/PR cannot be resolved untill something else is and removed waiting for changes This PR has been reviewed and changes are needed before merging labels Nov 25, 2018

Conengmo mentioned this pull request Nov 25, 2018

Fix missing and redundant and imports in __init__ #1029

Merged

Merge remote-tracking branch 'upstream/master' into GeoJsonTooltipUpdate

0d23dec

Conengmo merged commit f4d024d into python-visualization:master Dec 9, 2018

Conengmo removed the blocked This issue/PR cannot be resolved untill something else is label Dec 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GeoJsonTooltip warn for GeometryCollection #988

GeoJsonTooltip warn for GeometryCollection #988

jtbaker commented Oct 14, 2018 •

edited by Conengmo

Loading

jtbaker commented Oct 15, 2018

Conengmo commented Oct 16, 2018

Conengmo Oct 16, 2018

jtbaker Oct 16, 2018

Conengmo commented Oct 24, 2018

stickler-ci Oct 29, 2018

stickler-ci Oct 29, 2018

stickler-ci Oct 29, 2018

stickler-ci Oct 29, 2018

stickler-ci Oct 29, 2018

stickler-ci Oct 29, 2018

Conengmo commented Oct 30, 2018

Conengmo left a comment

Conengmo commented Dec 9, 2018

GeoJsonTooltip warn for GeometryCollection #988

GeoJsonTooltip warn for GeometryCollection #988

Conversation

jtbaker commented Oct 14, 2018 • edited by Conengmo Loading

jtbaker commented Oct 15, 2018

Conengmo commented Oct 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Conengmo commented Oct 24, 2018

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

Conengmo commented Oct 30, 2018

Conengmo left a comment

Choose a reason for hiding this comment

Conengmo commented Dec 9, 2018

jtbaker commented Oct 14, 2018 •

edited by Conengmo

Loading