Skip to content
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

Correct type annotations in Realtime #1960

Merged
merged 3 commits into from
Jun 18, 2024

Conversation

hansthen
Copy link
Collaborator

@hansthen hansthen commented Jun 2, 2024

This PR corrects a few type annotations for the Realtime plugin.

  1. Realtime superclass in leaflet is GeoJson (which is a subclass of featuregroup). In Folium I cannot make Realtime a subclass of GeoJson since GeoJson requires features to be present before rendering. I made it a subclass of FeatureGroup to more clearly document that features can be added to a Realtime layer.

  2. The container parameter for Realtime cannot just be any L.Layer. It must be a FeatureGroup or something that allows adding features.

I created type annotations to reflect this on the Folium side.

1. Realtime superclass in leaflet is GeoJson (which is a subclass
of featuregroup). In Folium I cannot make Realtime a subclass of
GeoJson since GeoJson requires features to be present before
rendering. I made it a subclass of FeatureGroup to more clearly
document that features can be added to a Realtime layer.

2. The container parameter for Realtime cannot just be any `L.Layer`.
It must be a `FeatureGroup` or something that allows adding features.

I created type annotations to reflect this on the Folium side.
@hansthen hansthen requested a review from Conengmo June 2, 2024 18:32
Copy link
Member

@Conengmo Conengmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small comment about updating the docstring, good to go afterwards!

folium/plugins/realtime.py Show resolved Hide resolved
Copy link
Member

@Conengmo Conengmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know why that test failed, maybe a hickup and we should try rerunning it? It's approved anyway.

@Conengmo
Copy link
Member

@ocefpaf is looking into that test failure in #1978, so no need to do that here fortunately. Maybe you can revert the latest commit, then merge it?

@ocefpaf
Copy link
Member

ocefpaf commented Jun 17, 2024

Keep merging things without worrying about that Windows failure. It is a problem with the dependencies in conda-forge and we are working to fix it upstream.

@hansthen hansthen merged commit 8bc6b02 into python-visualization:main Jun 18, 2024
12 checks passed
@@ -1,4 +1,5 @@
branca>=0.6.0
fiona
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hansthen this got merged with this added dependency in the requirements. I don’t think that’s right, since it’s not a required dependency. Could you maybe make a PR to revert this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants