less code

Author: raven-wing

docs/data-source.rst

@@ -272,9 +272,10 @@ a map with no ``marker_styles`` still renders, just with plain pins.
      "marker_styles": {
        "icon_field": "type_of_place",
        "color_field": "transparency",
+       "icon_provider": "phosphor",
        "icons": {
-         "big bridge": "https://cdn.example.com/bridge.svg",
-         "container": {"provider": "phosphor", "value": "shipping-container"}
+         "big bridge": "bridge",
+         "container": "shipping-container"
        },
        "colors": {
          "lacking": "#c62828",
@@ -292,17 +293,24 @@ a map with no ``marker_styles`` still renders, just with plain pins.
    only some of your points carry. A name that isn't declared there is ignored, and
    pins get no icon/color from it.
 
+``icon_provider``
+   Where your icons come from. One provider serves the whole ``icons`` table:
+
+   ``phosphor``
+      Entries are `Phosphor <https://phosphoricons.com/>`_ icon names in kebab-case
+      (e.g. ``"shipping-container"``), so you need not host SVGs yourself.
+   ``url``
+      Entries are URLs of SVGs you host.
+
+   Required whenever ``icons`` has anything in it. Naming a provider GoodMap does not
+   know stops the app from starting, so the mistake surfaces on deploy rather than as a
+   silently unstyled pin.
+
 ``icons``
-   Maps a value of ``icon_field`` to either a plain URL string, or
-   ``{"provider": "phosphor", "value": "<icon-name>"}`` to use a `Phosphor
-   <https://phosphoricons.com/>`_ icon by name instead of hosting your own SVG.
-   ``{"provider": "url", "value": "..."}`` is the plain string spelled out explicitly.
-
-   ``phosphor`` and ``url`` are the providers GoodMap knows; each one's URL is built
-   server-side, so the browser only ever receives finished URLs and a new provider needs
-   no frontend release. An entry GoodMap cannot make sense of — an unknown ``provider``,
-   a missing ``value`` — stops the app from starting, so the mistake surfaces on deploy
-   rather than as a silently unstyled pin.
+   Maps a value of ``icon_field`` to whatever ``icon_provider`` takes — an icon name for
+   ``phosphor``, a URL for ``url``. GoodMap turns these into finished URLs when the app
+   starts, so the browser never sees the provider and adding a new one needs no frontend
+   release.
 
 ``colors``
    Maps a value of ``color_field`` to a CSS color.

e2e-tests/e2e_test_data_initial.json

@@ -276,15 +276,10 @@
     "marker_styles": {
       "icon_field": "type_of_place",
       "color_field": "speed_limit",
+      "icon_provider": "phosphor",
       "icons": {
-        "big bridge": {
-          "provider": "phosphor",
-          "value": "bridge"
-        },
-        "small bridge": {
-          "provider": "phosphor",
-          "value": "footprints"
-        }
+        "big bridge": "bridge",
+        "small bridge": "footprints"
       },
       "colors": {
         "10": "#2e7d32",

goodmap/marker_styles.py

@@ -56,47 +56,33 @@ def resolve(self, value: str) -> str:
 }
 
 
-def _icon_url(entry: Any) -> str:
-    """The URL one marker_styles.icons entry stands for.
-
-    Args:
-        entry: A plain URL string - shorthand for the "url" provider - or a tagged
-            {"provider": <name in ICON_PROVIDERS>, "value": str} dict.
-
-    Returns:
-        The resolved URL.
-
-    Raises:
-        KeyError, TypeError: The entry is malformed. Deliberately not caught: bad
-            marker_styles config stops the app from starting, the same way a category
-            with no allowed values does (see data_models.location.create_location_model).
-    """
-    if isinstance(entry, str):
-        return ICON_PROVIDERS["url"].resolve(entry)
-    return ICON_PROVIDERS[entry["provider"]].resolve(entry["value"])
-
-
 def resolve_marker_styles(marker_styles: dict[str, Any]) -> dict[str, Any]:
     """Resolve marker_styles.icons into a flat {value: url} lookup table.
 
+    One "icon_provider" serves the whole table, so every entry is a plain value that
+    provider understands - a Phosphor icon name, a URL - rather than each one restating
+    which provider it came from.
+
     Args:
         marker_styles: Raw marker_styles config as returned by
-            goodmap.db.get_marker_styles(). May be empty or lack an "icons" key. Never
-            mutated - for the json backend this is the db's live in-memory config, so
-            resolving in place would rewrite what the deployment has stored.
+            goodmap.db.get_marker_styles(). Carries "icon_provider" (a name in
+            ICON_PROVIDERS) whenever it carries "icons". May be empty or lack both.
+            Never mutated - for the json backend this is the db's live in-memory config,
+            so resolving in place would rewrite what is stored.
 
     Returns:
         A new dict. "icons", if present, is replaced by a flat {value: url} map; every
         other key (icon_field, color_field, colors) is carried through untouched.
-        "colors" needs no resolving - it maps straight to CSS colors and never had a
-        tagged form.
+        "colors" needs no resolving - it maps straight to CSS colors, with no provider.
 
     Raises:
-        AttributeError, KeyError, TypeError: marker_styles.icons is malformed; see
-            _icon_url. Uncaught by design, so the app refuses to start.
+        KeyError, TypeError: "icon_provider" is missing or names a provider that does
+            not exist. Uncaught by design: bad config stops the app from starting, the
+            same way a category with no allowed values does (see
+            data_models.location.create_location_model).
     """
-    icons = marker_styles.get("icons")
-    if icons is None:
-        return dict(marker_styles)
-
-    return {**marker_styles, "icons": {key: _icon_url(entry) for key, entry in icons.items()}}
+    resolved = dict(marker_styles)
+    if icons := marker_styles.get("icons"):
+        provider = ICON_PROVIDERS[marker_styles["icon_provider"]]
+        resolved["icons"] = {key: provider.resolve(value) for key, value in icons.items()}
+    return resolved

tests/unit_tests/test_goodmap.py

@@ -1,6 +1,8 @@
 import importlib.metadata
 import io
+import json
 import os
+import re
 import sys
 import tempfile
 import types
@@ -132,9 +134,8 @@ def test_map_route_marker_styles():
                 "categories": {"type_of_place": ["parcel_locker", "container"]},
                 "marker_styles": {
                     "icon_field": "type_of_place",
-                    "icons": {
-                        "parcel_locker": "https://cdn.jsdelivr.net/npm/@phosphor-icons/core@2/assets/fill/package-fill.svg"
-                    },
+                    "icon_provider": "phosphor",
+                    "icons": {"parcel_locker": "package"},
                     "colors": {},
                 },
             },
@@ -170,6 +171,7 @@ def test_map_route_marker_styles_stay_in_step_with_the_api():
         "location_obligatory_fields": [["type_of_place", "str"]],
         "marker_styles": {
             "icon_field": "type_of_place",
+            "icon_provider": "url",
             "icons": {"parcel_locker": "https://cdn.example.com/package.svg"},
             "colors": {},
         },
@@ -193,15 +195,16 @@ def test_map_route_marker_styles_stay_in_step_with_the_api():
 
 
 def test_map_route_serves_icons_already_resolved_to_urls():
-    """window.MARKER_STYLES.icons is a flat {value: url} table: the tagged
-    {provider, value} form a data source may use is resolved at startup, so supporting a
-    new provider never needs a frontend release."""
+    """window.MARKER_STYLES.icons is a flat {value: url} table: the provider-specific
+    values a data source configures are resolved at startup, so supporting a new provider
+    never needs a frontend release."""
     data = {
         "site_content": {"pages": []},
         "location_obligatory_fields": [["type_of_place", "str"]],
         "marker_styles": {
             "icon_field": "type_of_place",
-            "icons": {"big bridge": {"provider": "phosphor", "value": "bridge"}},
+            "icon_provider": "phosphor",
+            "icons": {"big bridge": "bridge"},
             "colors": {},
         },
     }
@@ -218,11 +221,12 @@ def test_map_route_serves_icons_already_resolved_to_urls():
 
     response_text = app.test_client().get("/map").data.decode("utf-8")
 
-    assert (
-        "https://cdn.jsdelivr.net/npm/@phosphor-icons/core@2/assets/fill/bridge-fill.svg"
-        in response_text
-    )
-    assert "provider" not in response_text
+    match = re.search(r"window\.MARKER_STYLES\s*=\s*(.*?);", response_text)
+    assert match is not None
+    served = json.loads(match.group(1))
+    assert served["icons"] == {
+        "big bridge": "https://cdn.jsdelivr.net/npm/@phosphor-icons/core@2/assets/fill/bridge-fill.svg"
+    }
 
 
 def test_map_route_includes_photo_constraints():

tests/unit_tests/test_marker_styles.py

@@ -26,66 +26,74 @@ class SpriteProvider:
         def resolve(self, value):
             return f"https://sprites.example/{value}.svg"
 
-    styles = {"icons": {"big bridge": {"provider": "sprite", "value": "bridge"}}}
+    styles = {"icon_provider": "sprite", "icons": {"big bridge": "bridge"}}
 
     with mock.patch.dict(ICON_PROVIDERS, {"sprite": SpriteProvider()}):
         resolved = resolve_marker_styles(styles)
 
     assert resolved["icons"] == {"big bridge": "https://sprites.example/bridge.svg"}
 
 
-def test_resolves_phosphor_entry_to_cdn_url():
-    styles = {"icons": {"big bridge": {"provider": "phosphor", "value": "bridge"}}}
-
-    assert resolve_marker_styles(styles)["icons"] == {"big bridge": PHOSPHOR_BRIDGE_URL}
-
-
-def test_resolves_url_provider_entry_to_its_value():
-    styles = {"icons": {"container": {"provider": "url", "value": "https://e.example/c.svg"}}}
+def test_phosphor_provider_resolves_every_entry_in_the_table():
+    styles = {
+        "icon_provider": "phosphor",
+        "icons": {"big bridge": "bridge", "small bridge": "footprints"},
+    }
 
-    assert resolve_marker_styles(styles)["icons"] == {"container": "https://e.example/c.svg"}
+    assert resolve_marker_styles(styles)["icons"] == {
+        "big bridge": PHOSPHOR_BRIDGE_URL,
+        "small bridge": (
+            "https://cdn.jsdelivr.net/npm/@phosphor-icons/core@2/assets/fill/footprints-fill.svg"
+        ),
+    }
 
 
-def test_passes_plain_string_entry_through_unchanged():
-    styles = {"icons": {"container": "https://e.example/c.svg"}}
+def test_url_provider_serves_entries_the_deployment_hosts_itself():
+    styles = {"icon_provider": "url", "icons": {"container": "https://e.example/c.svg"}}
 
     assert resolve_marker_styles(styles)["icons"] == {"container": "https://e.example/c.svg"}
 
 
 @pytest.mark.parametrize(
-    "entry",
+    "styles",
     [
-        {"provider": "phosphorr", "value": "bridge"},
-        {"value": "bridge"},
-        {"provider": "phosphor"},
-        7,
-        None,
+        {"icon_provider": "phosphorr", "icons": {"big bridge": "bridge"}},
+        {"icons": {"big bridge": "bridge"}},
+        {"icon_provider": None, "icons": {"big bridge": "bridge"}},
     ],
-    ids=["unknown-provider", "no-provider", "no-value", "number", "null"],
+    ids=["unknown-provider", "no-provider", "null-provider"],
 )
-def test_malformed_entry_stops_the_app_from_starting(entry):
+def test_malformed_config_stops_the_app_from_starting(styles):
     """Bad marker_styles config is a deploy-time mistake, so it raises rather than
     quietly costing a pin its icon - the same stance create_location_model takes on a
     category with no allowed values."""
-    with pytest.raises((KeyError, TypeError, AttributeError)):
-        resolve_marker_styles({"icons": {"big bridge": entry}})
+    with pytest.raises((KeyError, TypeError)):
+        resolve_marker_styles(styles)
 
 
 def test_empty_marker_styles_stays_empty():
     assert resolve_marker_styles({}) == {}
 
 
-def test_missing_icons_key_is_not_invented():
-    assert resolve_marker_styles({"icon_field": "type_of_place"}) == {"icon_field": "type_of_place"}
+@pytest.mark.parametrize("icons", [{}, None], ids=["empty-table", "no-icons-key"])
+def test_nothing_to_resolve_needs_no_provider(icons):
+    """A deployment that styles pins by color alone never names an icon provider, so an
+    absent or empty table must not demand one."""
+    styles = {"icon_field": "type_of_place", "colors": {"10": "#2e7d32"}}
+    if icons is not None:
+        styles["icons"] = icons
+
+    assert resolve_marker_styles(styles) == styles
 
 
 def test_every_other_key_is_carried_through_untouched():
-    """colors maps straight to CSS colors and never had a tagged form, so it - like the
-    two field names - must survive resolution unchanged."""
+    """colors maps straight to CSS colors and has no provider, so it - like the two field
+    names - must survive resolution unchanged."""
     styles = {
         "icon_field": "type_of_place",
         "color_field": "speed_limit",
         "colors": {"10": "#2e7d32", "50": "#c62828"},
+        "icon_provider": "url",
         "icons": {"plain": "https://e.example/c.svg"},
     }
 
@@ -99,10 +107,7 @@ def test_every_other_key_is_carried_through_untouched():
 def test_does_not_mutate_the_config_it_was_given():
     """For the json backend this dict is the db's live in-memory config, so resolving in
     place would rewrite what the deployment has stored."""
-    styles = {
-        "icon_field": "type_of_place",
-        "icons": {"big bridge": {"provider": "phosphor", "value": "bridge"}},
-    }
+    styles = {"icon_provider": "phosphor", "icons": {"big bridge": "bridge"}}
     before = copy.deepcopy(styles)
     icons_before = styles["icons"]