fix __geo_interface__ styling #1935
Conversation
|
This one fixes the geopandas issue but I need to amend the docs for the pitfalls in the loop case and I also need to check if my simplistic solution is good enough and doesn't clash with the current styling options we have.
|
| @@ -237,6 +237,23 @@ colormap.add_to(m) | |||
| m | |||
| ``` | |||
|
|
|||
| > | |||
| > **Caveat** | |||
There was a problem hiding this comment.
@Conengmo do you have any suggestion to make this more prominent, like a red warning sign?
Also, I'd like to link the geopandas_and_geo_interface.html#Adding-style to the GeoPandas object suggestion below but I have no idea how to do that here.
There was a problem hiding this comment.
I think it is clear enough with the specified Caveat. It is a very tricky bug, but I also imagine it is not a very common software pattern to create style functions in a loop. (It took some ten years for this to be discovered.)
There was a problem hiding this comment.
Sadly, it is :-/
I recall (now b/c I forgot ;-) adding the geopandas alternative to escape that. We have a few issues reported and question on SO. Hopefully we can point folks to this paragraph instead from now on.
| @@ -643,6 +643,10 @@ class GeoJson(Layer): | |||
| .done({{ this.get_name() }}_add); | |||
| {%- endif %} | |||
|
|
|||
| {%- if not this.style %} | |||
There was a problem hiding this comment.
I checked all the docs/notebooks we have and none broke. If the user specifies a style_function, whatever is in the geojson will be ignored but, if nothing is specified and the geojson has style info, that will be used instead. I guess this is the best I could come up with but I'm open to suggestions.
There was a problem hiding this comment.
Is this change required to fix this bug? It seems like a new feature. If so perhaps it is better to separate this into a new PR.
Also, what happens if a Feature in the GeoJson does not have a style property? Will leaflet handle this correctly?
| @@ -643,6 +643,10 @@ class GeoJson(Layer): | |||
| .done({{ this.get_name() }}_add); | |||
| {%- endif %} | |||
|
|
|||
| {%- if not this.style %} | |||
There was a problem hiding this comment.
Is this change required to fix this bug? It seems like a new feature. If so perhaps it is better to separate this into a new PR.
Also, what happens if a Feature in the GeoJson does not have a style property? Will leaflet handle this correctly?
There was a problem hiding this comment.
Are these changes required for the issue at hand? If not perhaps it is cleaner to open a separate PR.
There was a problem hiding this comment.
They are required to fix the styling for the geopandas case. This fixes a regression introduced during the GeoJson refactoring. I prefer to keep them both in the same PR b/c they are tied together by the same issue, even though they have different origins.
Closes #1932.
__geo_interface__object) styling regression.style_functionin a loop.