Skip to content

allow passing TileLayer to Map#1624

Merged
ocefpaf merged 2 commits intopython-visualization:mainfrom
Conengmo:pass-tilelayer-to-map
Nov 8, 2022
Merged

allow passing TileLayer to Map#1624
ocefpaf merged 2 commits intopython-visualization:mainfrom
Conengmo:pass-tilelayer-to-map

Conversation

@Conengmo
Copy link
Member

@Conengmo Conengmo commented Oct 7, 2022

Looking at #1316 and #1033, this seemed like an obvious improvement that doesn't actually increase the parameter footprint of the Map class.

With this change a user doesn't have to do the following:

m = Map(tiles=None)
TileLayer().add_to(m)

but can do this directly:

m = Map(tiles=TileLayer())

Discussion

I still think we should be careful to add more parameters to Map, to prevent it from becoming a god-class. If Map has the same parameters as TileLayer, we are doing something wrong. I think we should stick with Map having the tiles argument as a convenience, and have users use TileLayer when they want to do more advanced things.

Maybe a way to make this more clear is by allowing the tiles argument of Map to only be one of the pre-defined string arguments or a TileLayer, but not a custom url. Users wanting to use a custom url are thus forced to use TileLayer. That way we could also scrap the attr argument of Map.

Since that would mean breaking changes for some users, I don't think we should actually go ahead with that.

@ocefpaf
Copy link
Member

ocefpaf commented Oct 10, 2022

Looks like a great improvement IMO and it is not really an extra parameter we are adding, just a valid input for an existing one.

@Conengmo Conengmo added the ready PR is ready for merging label Nov 7, 2022
@ocefpaf ocefpaf force-pushed the pass-tilelayer-to-map branch from 39b9ab3 to d8ed353 Compare November 8, 2022 19:04
@ocefpaf ocefpaf merged commit 91f34f9 into python-visualization:main Nov 8, 2022
@Conengmo Conengmo deleted the pass-tilelayer-to-map branch November 9, 2022 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready PR is ready for merging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants