Bringing folium closer to Leaflet

[themiurgo]

A one-line change to put features in the main namespace and bring folium closer to Leaflet. No need to import folium.features.

import folium

mymap = folium.Map()
folium.Marker.add_to(my_map)

[ocefpaf]

👍

@BibMartin I am assigning you to this. If you are busy just sent it back to me.

[BibMartin]

bc0d638

I'm not very comfortable with the import *. First because it is forbidden in my narrow simplified python world ; but also because it will bring to namespace all utility variables such as Template, text_type, mercator_transform, ...

I know it's more heavier to maintain, but could we imagine bringing explicitly objects with

from .element import Element, Figure, JavascriptLink, CssLink, Div, MacroElement
from .map import Map, TileLayer, Icon, Marker, Popup
from .features import (WmsTileLayer, RegularPolygonMarker, Vega, GeoJson,
    TopoJson, GeoJsonStyle, ColorScale, MarkerCluster,
    DivIcon, CircleMarker, LatLngPopup, ClickForMarker,
    PolyLine, MultiPolyLine, ImageOverlay)

?

fc0aa4d

I'm okay ; provided that Map is not used.

8ec2d23

I'm okay in the principle.
But if we create a new add_children method (for adding several children) and an add_to one, do we still need a third add_child method ? At the cost of changing it's name ; shouldn't we hide it in '_add_child` to avoid having 3 similar methods in the API ?

[ocefpaf]

👎 for the * import

fc0aa4d I am pep8ing the code trying to find things like that. Expect a PR soon.

At the cost of changing it's name ; shouldn't we hide it in '_add_child` to avoid having 3 similar methods in the API?

Yes please.

[themiurgo]

I'm not very comfortable with the import *. First because it is forbidden in my narrow simplified python world ; but also because it will bring to namespace all utility variables such as Template, text_type, mercator_transform, ...

Sorry, I got lazy and the import * slipped without thinking. 😄

I'm okay ; provided that Map is not used.

It's not used, but it might be worth in the future change its name to avoid having two objects with the same name in the same library. They do different things.

I'm okay in the principle.
But if we create a new add_children method (for adding several children) and an add_to one, do we still need a third add_child method ? At the cost of changing it's name ; shouldn't we hide it in '_add_child` to avoid having 3 similar methods in the API ?

Then how about we go "full Leaflet" and keep only add_to? I can't think of any scenarios where you would want to add a list of features, without having a loop. In order to create a list of features you probably have already done a loop beforehand.

[ocefpaf]

Then how about we go "full Leaflet" and keep only add_to?

Let's be careful with "full Leaflet." I don't won't folium code to look like Javanesc 😉

Jokes aside @themiurgo is right and add_to() makes more sense. 👍

[ocefpaf]

@themiurgo can you rebase this please?

[themiurgo]

Rebasing this was a huge pain. If this tests ok and looks ok please merge soon, I don't want to go through this again. 😀

[ocefpaf]

I don't want to go through this again

Apologies to make you go thorough t.hat painful process. In cases like this it might be worth closing the PR and opening a new one

BTW maybe something happened in the rebase. I am having trouble following the changes. @themiurgo can you take a look if what you want is still there?

[themiurgo]

Yeah, I have the exact changes saved and tagged, it's just been tricky to rebase them for some reason. Will do this in the weekend.

[ocefpaf]

No problem. Thanks!

[BibMartin]

@themiurgo
I'm coming back to this PR (sorry it's late).
It seems there's few modification left after the rebase :

• As for the namespace part, it has been done by Make features accessible from root #258
• As for renaming add_children to add_child, it seems that only the notebook Features.ipynb is still affected. Though, it needs to be rewritten anyway after all API changes.

[ocefpaf]

Closing this. I believe we address all the points raised here. If not we can re-open it.