Skip to content

Allow users to add new Control classes without implementing a plugin #2122

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

Merged
merged 4 commits into from
Apr 22, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions folium/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
ClickForLatLng,
ClickForMarker,
ColorLine,
Control,
CustomIcon,
DivIcon,
GeoJson,
Expand Down Expand Up @@ -64,6 +65,7 @@
"ClickForLatLng",
"ColorLine",
"ColorMap",
"Control",
"CssLink",
"CustomIcon",
"Div",
Expand Down
74 changes: 72 additions & 2 deletions folium/features.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,18 @@
import json
import operator
import warnings
from typing import Any, Callable, Dict, Iterable, List, Optional, Sequence, Tuple, Union
from typing import (
Any,
Callable,
Dict,
Iterable,
List,
Optional,
Sequence,
Tuple,
Union,
get_args,
)

import numpy as np
import requests
Expand All @@ -33,6 +44,7 @@
TypeJsonValue,
TypeLine,
TypePathOptions,
TypePosition,
_parse_size,
escape_backticks,
get_bounds,
Expand Down Expand Up @@ -1813,7 +1825,7 @@ def __init__(self, popup: Union[IFrame, Html, str, None] = None):
if isinstance(popup, Element):
popup = popup.render()
if popup:
self.popup = "`" + escape_backticks(popup) + "`"
self.popup = "`" + escape_backticks(popup) + "`" # type: ignore
else:
self.popup = '"Latitude: " + lat + "<br>Longitude: " + lng '

Expand Down Expand Up @@ -1992,3 +2004,61 @@ def __init__(
out.setdefault(cm(color), []).append([[lat1, lng1], [lat2, lng2]])
for key, val in out.items():
self.add_child(PolyLine(val, color=key, weight=weight, opacity=opacity))


class Control(JSCSSMixin, MacroElement):
"""
Add a Leaflet Control object to the map

Parameters
----------
control: str
The javascript class name of the control to be rendered.
position: str
One of "bottomright", "bottomleft", "topright", "topleft"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One problem that we have in folium templates are the silent erros when passing keywords to it. For example, if someone types bottonright instead of bottomright, it will take user a while to figure out what is wrong.

Maybe we should check for this 4 valid kw? I know this doesn't solve it for all templates, but in this case it may be small enough that it is worth it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean as a static typing check or as an assert?

Copy link
Member

@ocefpaf ocefpaf Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid assert use outside of tests.

I'm old school but here is what I would do:

  1. make position a kw argument, not an arg;
  2. Sanitize it with a position.lower() to validate names copied from leaflet JS docs;
  3. Check if the sanitized positions is in a valid list with those 4 names and raise a ValueError if not.

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see we have been inconsistent in banning assert :-)


Examples
--------

>>> import folium
>>> from folium.features import Control, Marker
>>> from folium.plugins import Geocoder

>>> m = folium.Map(
... location=[46.603354, 1.8883335], attr=None, zoom_control=False, zoom_start=5
... )
>>> Control("Zoom", position="topleft").add_to(m)
"""

_template = Template(
"""
{% macro script(this, kwargs) %}
var {{ this.get_name() }} = new L.Control.{{this._name}}(
{% for arg in this.args %}
{{ arg | tojavascript }},
{% endfor %}
{{ this.options|tojavascript }}
).addTo({{ this._parent.get_name() }});
{% endmacro %}
"""
)

def __init__(
self,
control: Optional[str] = None,
*args,
position: Optional[TypePosition] = None,
**kwargs,
):
super().__init__()
if control:
self._name = control

if position is not None:
position = position.lower() # type: ignore
if position not in (args := get_args(TypePosition)):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I was a Fortran programmer in my early days, type checking is not my forte. (Heck, I moved to Python to get away from this ;-p)

My crude review tells me that this is equivalent to what I described in my review comment, right? If so, merge away!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote my first program for money in RPG :-) which is a fairly obscure language almost as old as Fortran. Whitespace is significant to a fault.

     C* PARAMETER LIST
     C           *ENTRY    PLIST
     C                     PARM           DATEIN  80
     C* PROGRAM START
     C           DATEIN    DIV  100       DATEIN
     C                     MVR            DAY     20
     C           DATEIN    DIV  100       DATEIN
     C                     MVR            MONTH   20
     C                     Z-ADDDATEIN    YEAR    40
     C* SHOW RESULTS
     C           YEAR      DSPLY
     C           MONTH     DSPLY
     C           DAY       DSPLY
     C                     SETON                         LR
     C* EXECUTION EXAMPLE
     C* CALL SEPDATE PARM(X'019960531F')

I added one more test.

raise TypeError(f"position must be one of {args}")
kwargs["position"] = position

self.args = args
self.options = remove_empty(**kwargs)
2 changes: 2 additions & 0 deletions folium/utilities.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
Iterable,
Iterator,
List,
Literal,
Optional,
Sequence,
Tuple,
Expand Down Expand Up @@ -57,6 +58,7 @@
TypeBoundsReturn = List[List[Optional[float]]]

TypeContainer = Union[Figure, Div, "Popup"]
TypePosition = Literal["bottomright", "bottomleft", "topright", "topleft"]


_VALID_URLS = set(uses_relative + uses_netloc + uses_params)
Expand Down
26 changes: 26 additions & 0 deletions tests/test_folium.py
Original file line number Diff line number Diff line change
Expand Up @@ -501,3 +501,29 @@ def test_json_request(self):
np.testing.assert_allclose(
bounds, [[18.948267, -178.123152], [71.351633, 173.304726]]
)

def test_control_typecheck(self):
m = folium.Map(
location=[39.949610, -75.150282], zoom_start=5, zoom_control=False
)
tiles = TileLayer(
tiles="OpenStreetMap",
show=False,
control=False,
)
tiles.add_to(m)

with pytest.raises(TypeError) as excinfo:
minimap = folium.Control("MiniMap", tiles, position="downunder")
minimap.add_js_link(
"minimap_js",
"https://cdnjs.cloudflare.com/ajax/libs/leaflet-minimap/3.6.1/Control.MiniMap.min.js",
)
minimap.add_css_link(
"minimap_css",
"https://cdnjs.cloudflare.com/ajax/libs/leaflet-minimap/3.6.1/Control.MiniMap.css",
)
minimap.add_to(m)
assert "position must be one of ('bottomright', 'bottomleft'" in str(
excinfo.value
)