Adds PolyLineOffset Plugin #1091
Conversation
There was a problem hiding this comment.
Hi @FabeG, thanks for the great PR! I added some comments, hope you can have a look.
About the example notebook:
- in the first example you have some values commented, you can remove them then I guess
- using
llas a variable name is very confusing :p are those L's or ones? - the bus example is really cool. Very minor detail:
for j in range(len(lines_on_segment))is not very Pythonic, you could use insteadfor j, line in enumerate(lines_on_segment). But that's just nitty picky stuff.
folium/plugins/polyline_offset.py
Outdated
|
|
||
| def __init__(self, locations, popup=None, tooltip=None, **kwargs): | ||
| super(PolyLineOffset, self).__init__( | ||
| locations=locations, popup=popup, tooltip=tooltip |
There was a problem hiding this comment.
I'm hesitating whether we should not let the option handling be done by PolyLine. And then only add the offset argument in this class. I would like this plugin not copy code from PolyLine.
But then you would have to load the json, add the offset, and dump it again. Not doing json.dumps until render time is something I want to tackle in another PR.
There was a problem hiding this comment.
Taking advantage of your last PR (#1101, using the jinja2 tojson filter) avoiding the json.dumps, I reduced a little the code. Let me know if it seems better for you
| """ | ||
| ) # noqa | ||
|
|
||
| expected_rendered = tmpl.render(this=polylineoffset) |
There was a problem hiding this comment.
Cool that you added a test! I'm not sure if it's verifying that an offset was added though? If offset is not in the options, will the test fail?
There was a problem hiding this comment.
I added a test for this specific case (PolyLineOffset without offset in the options: in such case, offset takes its default value (0) and looks like a normal PolyLine).
Conengmo
added
waiting for changes
Conengmo
added
waiting for review
- changed tests taking into account GH-1101 - corrected error in Jupyter kernel name.
|
The failed CI build seems to come from the |
|
This is good stuff @FabeG! A very neat plugin implementation with a great example notebook. Indeed the Travis build failed on something unrelated, I'll restart it and see if we can get it working. After that this is good to go! |
Conengmo
added
ready
|
Thanks to your nice comments and your review @Conengmo ! |
|
No, I was waiting for the tests to finish and then forgot about this :p But the checks have passed so let's merge this thing! |
|
🚀 |
See https://github.com/bbecquet/Leaflet.PolylineOffset