-
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
Make Folium follow the Leaflet class hierarchy more closely #1941
Comments
Interesting idea! I'd love to discuss this further. I'll give some thoughts, counter arguments really, and I hope we can talk about it. I don't think following the Leaflet class hierarchy in itself should be a goal, my question would be what benefit it would bring. So where it brings us something, probably mostly less duplicate code, then it makes sense. But Python is different from Javascript, plus we have an existing code base to deal with, so it may be we choose to make different abstractions than Leaflet has. The class hierarchy in Branca + Folium is already pretty complex, and even I with how much experience I have with these projects sometimes have trouble grokking how things inherit. Or maybe that's just me? ;) I'd opt for a less complex class structure if possible. I think this is a common issue in software, see 'multiple inheritance'. One radical thought, also applies to #1942: we're not constrained to Folium. We can make a fork (New Folium?) and start over, build it just the way you envision it. If that makes things much better in terms of features or maintainability, that's great! We can port things back, or just point people to the new version. Some general thoughts, hope you find it interesting. |
Thanks for your reply! Your comments are really welcome. Counter arguments make for better decisions. I agree, following the Leaflet class hierarchy should not be goal in itself. I also think some things can become clearer if we follow the Leaflet class hierarchy. I think there is a satisfying simplicity to the Leaflet class diagram. I find it very intuitive (ymmv). https://leafletjs.com/examples/extending/class-diagram.html. As a new Folium user coming from My main goal with following the Leaflet class hierarchy more closely is not to reduce code, but to give end-users capabilities they would not have without extending Folium itself. By introducing
My hope is that by giving users more power, we reduce the maintenance burden on ourselves. In general I would not want to fork Folium. I really love the library as is. I would not want to compete with it. I suggest the following: I work out the three different proposals in separate pull requests. You can then judge each proposal to see if it improves the clarity of the class hierarchy. Is that a good way forward? |
And you both are authors/owners of folium now 😜, no need to fork unless you have a plan to sunset it. |
Sure, let's see the ideas! Maybe when you make PR's to save yourself some time, don't apply it on everything yet. |
Is your feature request related to a problem? Please describe.
I find I sometimes need to patch or extend Folium classes. This is to access Leaflet functionality that would be accessible directly from javascript. Starting as a Leaflet user before moving to Folium I find this annoying at times.
Describe the solution you'd like
The functionality I miss resides in a few classes in the Leaflet class hierarchy. Notably:
My proposal would be to implement these classes in Folium.
folium.Class will provide the
include
method, which makes it really easy to customize any Leaflet object.folium.Evented provides the possibility to add javascript event handlers using
on(event=JsCode()
.folium.Control factors out the common code for controls.
folium.Class will be a subclass of branca.MacroElement. It will be the parent class for all Leaflet based classes in Folium.
folium.Evented would become a superclass of folium.Layer.
folium.Control will become the superclass for the core folium controls and also for the plugins that now derive from MacroElement and implement a leaflet Conrol.
These changes should be backwards compatible for most uses.
There is already a PR for adding an event handler and for a Leaflet.Control object, #1903 and #1902. If you agree this proposal is a good idea, I will rewrite those PRs, to make them align with the class hierarchy.
Describe alternatives you've considered
So far I have been copying and modifying Folium classes to get what I want.
Implementation
I am willing to implement this.
The text was updated successfully, but these errors were encountered: