-
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
Add leaflet-realtime plugin #1848
Conversation
Any feedback? If you currently have no time for a proper review let me know. |
Hi Hans, good that you're asking. I'm busy at work with the end of the year, but I'll take a proper look after this week is over! Thanks for your 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.
This is a nice plugin to have. I think it's okay to not support the container
argument in this first version, we can always add that in a next one.
This feature could really use an example in our documentation. The demo on https://github.com/perliedman/leaflet-realtime is actually broken for me :( Can we add a good example to our documentation? Ideally it would not be dependent on a flaky external data service... We've had that in the past and it kept breaking our tests on random moments.
The examples in our documentation also function as tests. But separate tests are welcome as well, though not a hard requirement.
folium/plugins/realtime.py
Outdated
) | ||
] | ||
|
||
def __init__(self, src, **kwargs): |
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'd rather explicitly name all the arguments here that are also in the docstring. That way it's more transparent for users and IDE's what the arguments of this function are. We can add type hints for each argument.
The JS functions are already added to options
separately in the JS part, so that can still happen there.
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 have made the arguments from the docstring explicit. I made them default None
because I did not want them to shadow the defaults from leaflet-realtime
.
I will work on better documentation. Would it be okay to add a static geojson to the repo? It would not really be realtime, but at least it will not break the tests. |
Please forgive my ignorance, as I have no clue what I'm talking about. Could you use something like that for testing? def get_random_location_nearby(base_location, max_deviation=0.02):
"""
Generate a random location near the base location.
"""
lon, lat = base_location
new_lon = lon + random.uniform(-max_deviation, max_deviation)
new_lat = lat + random.uniform(-max_deviation, max_deviation)
return [new_lon, new_lat] Thank you for your endurance in implementing! |
I’m on mobile so only responding to the test data thing: we have a repo with example data at https://github.com/python-visualization/folium-example-data. You can use a file from there or add a new one if you want. Some of our existing documentation already uses files from there. I understand we cannot have a dynamic example, that’s okay then :) I’ll check out the rest later! |
I don't think it would work. The leaflet-realtime plugin typically downloads a geojson object from a url. I can make it point to a url inside the github repo. Alternatively, we could spin up a small webserver to serve geojson during testing, but that seems like overkill. |
Okay thanks. |
Interesting thought. We don’t have a webserver we can run things on. We do host our documentation on GitHub Pages, could that include something like that? |
d1a7b5a
to
a2af509
Compare
I updated the example in the docstring so it does not use some random datasource, but the New York subway stations from the example-data. Also, I added documentation with some more examples (one of them tracking the International Space Station). |
Looks good! This is an interesting plugin to have. Especially like the option to add a custom source, I was playing around a bit with that, lots of flexibility. I did a test with random lat lons and that worked perfectly. Really good documentation you added also. Love the multiple examples. Rather than doing another round of reviews to dot the i's, I took the liberty of making some changes on your branch myself. Mostly to get the documentation right. You can see the commits, I'll elaborate here as well:
With this I'm ready to merge this PR. Do you want to take a look, see if you agree with these changes, or if you have any comments or issues? Let me know. |
I am fine with your changes. Thank you for the improvements. |
Perfect! Thanks for your contribution @hansthen! |
* Implemented the leaflet-realtime plugin Based on: https://github.com/perliedman/leaflet-realtime * Fix for failing pre-commit hooks in origin * Updated after review comments * Add documentation for the realtime plugin * Also update TOC * Fix layout * remove noqa * don't use `options` var name * use default arguments, add typing * Update JsCode docstring for in docs * Add JsCode to docs * remove parameters from docs * slight tweaks to docs * import JsCode in init --------- Co-authored-by: Frank <[email protected]>
Based on: https://github.com/perliedman/leaflet-realtime
A current limitation of folium is that it is not possible to perform dynamic updates to an existing map (see e.g. #848, #1581, #1233). A way to overcome that limitation is to use the leaflet-realtime plugin. This plugin allows polling for a url that provides geojson data.
I saw several suggestions in the past to implement this plugin in folium (see #735 and #127), so I decided to give it a go.
I did not test the full plugin functionality yet; just the basic
url
lookup of a static geojson datasource. Most notably, thecontainer
parameter is not implemented. I am unsure what the best way would be to implement this. Should it refer to a folium object or should it refer to a javascript object?If there is any interest in this plugin, I can add more tests and implement the
container
parameter.I also added a
JsCode
utility wrapper around strings. The idea was stolen from several streamlit plugins, that use a similar class to pass javascript functions from python to javascript. I mainly used it because I was too lazy to implement all the function parameters fromL.realtime
andL.geojson
in the template.