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

Geocoder can be generalized to work with non-Nominatim providers #1851

Closed
prusswan opened this issue Dec 24, 2023 · 6 comments
Closed

Geocoder can be generalized to work with non-Nominatim providers #1851

prusswan opened this issue Dec 24, 2023 · 6 comments
Labels
enhancement Feature request or idea about how to make folium better plugin This issue/PR is about an existing or new plugin

Comments

@prusswan
Copy link
Contributor

Is your feature request related to a problem? Please describe.
The Geocoder plugin for folium is built on top of of the Javascript geocoder (https://github.com/perliedman/leaflet-control-geocoder) but does not expose all functionality to folium users (e.g. it defaults to Nominatim provider with no option to change)

Describe the solution you'd like
Expanding the Geocoder plugin to support all providers listed at: https://github.com/perliedman/leaflet-control-geocoder#leaflet-control-geocoder-- and possibly user-defined providers

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

N/A

Additional context
Add any other context or screenshots about the feature request here.

Implementation
folium is maintained by volunteers. Can you help make a PR if we want to implement this feature?

Yes. I have some related work done but would need help to make sure the implementation is generalized and fit with folium design.

@prusswan prusswan changed the title Geocoder can be generalized to work with non-Nominatim provider Geocoder can be generalized to work with non-Nominatim providers Dec 24, 2023
@Conengmo
Copy link
Member

Hi Prusswan,

Is this a usecase you need? I'm open to expanding the Geocoder class to provide more of the underlying options. I do hope we can do that in a sane and minimal way though, so we don't take on too much extra maintenance load. If you have already related work done, could you show what you have in mind? Just open a PR if you like, I can take a look.

@Conengmo Conengmo added enhancement Feature request or idea about how to make folium better plugin This issue/PR is about an existing or new plugin labels Dec 24, 2023
@prusswan
Copy link
Contributor Author

prusswan commented Dec 24, 2023

@Conengmo thanks. See #1852, I have tested with 4 providers including Nominatim

@Conengmo
Copy link
Member

Conengmo commented Jan 2, 2024

Thanks @prusswan! I took a look, and will continue the discussion here. I'll make a judgement call and say that this is too much custom Javascript code to accept in Folium. Meaning we cannot take on that maintenance burden.

Though it is cool work that you did! I suggest you host it as a Folium plugin yourself, and when you do we can link to it. We have done this with others before, see https://github.com/python-visualization/folium#packages-and-plugins.

Is this acceptable to you? Or do you have other thoughts or ideas? Love to hear them.

@prusswan
Copy link
Contributor Author

prusswan commented Jan 2, 2024

@Conengmo I understand your reasoning, but I don't think I would want to maintain a plugin like this when 1) it is effectively a single class, expanded replacement of the "official" Geocoder 2) part of the functionality should have been supported by the Geocoder (as raised in this issue) 3) custom Javascript is unavoidable given the nature of the plugin (to be executed on client-side), and yet folium is designed with enough flexibility (a good thing) to accommodate such changes (I tried something similar with ipyleaflet, but could not see an "easy" way due to its design)

One way to reduce the custom Javascript is to exclude the functionality for user-defined geocoding providers and just allow the option to use non-Nominatim providers (and to make maintenance more manageable, just mention the providers that have been tested specifically in the docs, leaving the rest "open")

@Conengmo
Copy link
Member

Conengmo commented Jan 3, 2024

Thanks for your response, that sounds like a very reasonable compromise. Since the custom template is most of the new code, that does reduce the maintenance load significantly. Let's continue with that PR then based on your suggestion. I'll also add some comments there so you can possibly address everything in one go.

@Conengmo
Copy link
Member

Since #1852 was merged I'll close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request or idea about how to make folium better plugin This issue/PR is about an existing or new plugin
Projects
None yet
Development

No branches or pull requests

2 participants