-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds PolyLineOffset Plugin #1091
Adds PolyLineOffset Plugin #1091
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
ll
as 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice :)
""" | ||
) # noqa | ||
|
||
expected_rendered = tmpl.render(this=polylineoffset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
- 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! |
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:/bbecquet/Leaflet.PolylineOffset