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

TileLayer: max_native_zoom cannot be null #2044

Merged
merged 3 commits into from
Dec 12, 2024

Conversation

Conengmo
Copy link
Member

@Conengmo Conengmo commented Dec 11, 2024

Closes #2043.

In #2029 we changed the way we parse options. Before, in parse_options, None values would be discarded. In the tojavascript filter this doesn't happen. This leads to different outcomes, and issues like some tile layers not rendering.

I first looked at removing all None values like before, but that seems heavy handed and might lead to other issues. Instead, do a targeted fix that just makes sure max_native_zoom always has an integer value. If it's not available, we can set it to max_zoom, which has the same effect as not including it at all.

@Conengmo Conengmo changed the title remove null values from options TileLayer: max_native_zoom cannot be null Dec 11, 2024
@Conengmo Conengmo marked this pull request as ready for review December 11, 2024 10:02
@Conengmo
Copy link
Member Author

@hansthen could you maybe take a look, see if you agree with this approach?

@Conengmo Conengmo requested a review from hansthen December 11, 2024 10:19
@hansthen
Copy link
Collaborator

I agree with your targeted approach for now.

I also feel that filtering None may cause other issues, because None or null is simply valid json. It felt more natural to do this during parse_options, because that code was scoped more limited.

Just to think out loud about possible alternatives.

The root cause is that in Folium arguments are statically typed, whereas in Leaflet they are not. Static typing has its advantages, but it removes the possibility to not pass an argument if we use None as a sentinel value.

Static typing cannot distinguish between:
fun(arg=None) and fun().

We could use a separate default argument as a sentinel value. E.g.

# define a sentinel
unset=object()

class ABC:
   def __init__(self, arg1=unset, arg2=unset):

We could then create a new function parse_options that will discard unset values. However, that is way more work and I am not sure it is worth it. Probably be the best would be to continue as is and simply ensure all __init__ methods have sensible default values that agree with their Leaflet counterparts.

@Conengmo
Copy link
Member Author

Thanks for the review! Your thinking here makes a lot of sense.

Probably be the best would be to continue as is and simply ensure all init methods have sensible default values that agree with their Leaflet counterparts.

I agree, and hope there aren't that many cases where this is now an issue. If so, it's nice to know we have other ideas on hand.

I'll go ahead and merge this, then make a v0.19.1 release.

@Conengmo Conengmo merged commit e299ec2 into python-visualization:main Dec 12, 2024
11 checks passed
@Conengmo Conengmo deleted the fix-remove-null-options branch December 12, 2024 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid Zoom for TileLayer in version 0.19.0
2 participants