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.
|
| ``` | ||
|
|
||
| > | ||
| > **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.
| .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?
| .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.