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

allow passing TileLayer to Map #1624

Merged
merged 2 commits into from
Nov 8, 2022

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 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