python-visualization / folium

Python Data. Leaflet.js Maps.
https://python-visualization.github.io/folium/
MIT License
6.86k stars 2.22k forks source link

There are two `Map` class in the code #247

Closed BibMartin closed 8 years ago

BibMartin commented 8 years ago

An issue to give more visibility to @themiurgo's remark from https://github.com/themiurgo/folium/commit/2300c2e2b375c607ca0f52ac95d69b65a4f2e697

@themiurgo

@BibMartin there seems to be a name clash for map.Map vs folium.Map. This has not to do with this PR but I discovered it because it was raising errors, not finding methods like add_wms_layer and similar. Is there's a reason why we have two different Map objects in two different modules?

In my mind, map.Map is the real true object we want to have and maintain. I just let folium.Map inherit from it to keep backward compatibility, and host deprecated methods. I imagine that folium.Map's methods will either move to map.Map or disapear.

ocefpaf commented 8 years ago

I am not sure I get the problem. (Probably because I haven't been paying attention as I should.)

That being said. I do not have any issues keeping folium.map.Map as "the one," but I would like to access it from the folium.<namespace> too.

themiurgo commented 8 years ago

The problem is now that a folium.Map (the old, legacy object) is inheriting from folium.map.Map (the new object, the one we want to keep in the long run). Having different, similar objects under the same name causes confusion and can determine name clashes.

Two solutions I think of:

ocefpaf commented 8 years ago

Having different, similar objects under the same name causes confusion and can determine name clashes.

Indeed. We have to fix that.

  • We can rename folium.map.Map -> folium.map.LegacyMap and have folium.map.Map inherit from it. When we want to drop the deprecated methods we remove the inheritance.
  • We merge the objects (keeping deprecated methods together in the same section) and when we want to drop the deprecated methods we just delete their lines. :smile:

Not sure which one is more appealing. I will leave the decision to @BibMartin.

BibMartin commented 8 years ago

@ocefpaf

That being said. I do not have any issues keeping folium.map.Map as "the one," but I would like to access it from the folium. too.

provided that folium.Map = folium.folium.Map inherits from folium.map.Map, you access it in the namespace (with more methods though).

  • We can rename folium.map.Map -> folium.map.LegacyMap and have folium.map.Map inherit from it. When we want to drop the deprecated methods we remove the inheritance.
  • We merge the objects (keeping deprecated methods together in the same section) and when we want to drop the deprecated methods we just delete their lines. :smile:

I would be in favor of the second solution because folium.map.Map is the future Map method. But it may force us to merge map.py and features.py, provided that the import stack is folium imports features ; features imports map. My idea on this point was to separate the core of maps and the additional features ; if we want to keep methods from folium.Map it means we want to incorporate the additional features in the base object.

ocefpaf commented 8 years ago

if we want to keep methods from folium.Map it means we want to incorporate the additional features in the base object.

Do you think that is a bad idea? What would be the downside?

ocefpaf commented 8 years ago

@BibMartin can you tag with v0.2.0 or v0.2.1 depending on the amount / importance.

BibMartin commented 8 years ago

Fixed in #329

themiurgo commented 8 years ago

Well done!