fastkml icon indicating copy to clipboard operation
fastkml copied to clipboard

Broaden `Change` registry coverage and tighten `_UpdateAction` typing

Open cleder opened this issue 3 months ago • 0 comments

In PR #479 (“Add Update to NetworkLinkControl”), we merged initial support for Update, Create, Delete, and Change, along with registry wiring and tests. During review, @hirohira9119 raised two follow-ups that aren’t yet addressed, but would improve both correctness and type safety.

1. Expand Change registry to all KML object types

Right now, the registry entry for Change only supports a limited set of feature-like classes:

# Change can contain any KML object (AbstractObjectGroup).
# For now, we support the common feature types.
# In the future, this could be expanded to include Style, StyleMap, etc.
_change_node_name = (
    "Folder,Placemark,Document,GroundOverlay,PhotoOverlay,ScreenOverlay,NetworkLink"
)
_change_classes = (
    Document,
    Folder,
    Placemark,
    GroundOverlay,
    PhotoOverlay,
    ScreenOverlay,
    NetworkLink,
)

As @hirohira9119 pointed out, this implementation is stricter than the KML schema’s AbstractObjectGroup and can’t represent valid KML where Change operates on styles, geometry, time primitives, etc. They demonstrated that we can mechanically derive the full concrete set of objects from _BaseObject and provided an example mapping of _change_node_name/_change_classes that covers all such types.([GitHub][1])

Follow-up work:

  • Expand _change_node_name and _change_classes to include the full set of non-abstract KML classes derived from _BaseObject, as in @hirohira9119’s comment.

  • Consider whether we should:

    • keep the explicit long list as-is, or
    • introduce a helper that builds it from the registry / class hierarchy at import time (with caching to avoid performance issues).
  • Add tests that confirm Change can handle non-feature objects like Style, StyleMap, geometry classes, and time primitives (round-trip XML).

2. Make _UpdateAction generic for better typing

The base _UpdateAction currently stores objects as a list of _XMLObject, and the concrete Create / Delete / Change subclasses don’t narrow this type. That means type checkers can’t enforce which kinds of objects belong in which operation, even though the registry and KML schema do have those constraints.

@hirohira9119 suggested making _UpdateAction generic over a type parameter T bound to _XMLObject, and then parameterizing the concrete actions accordingly:([GitHub][2])

  • _UpdateAction[T] – base class with objects: list[T]
  • Create(_UpdateAction["Document | Folder"])
  • Delete(_UpdateAction["Placemark | Folder | Document | GroundOverlay | ScreenOverlay"])
  • Change(_UpdateAction[<appropriate union of KML objects>])

Follow-up work:

  • Introduce TypeVar("T", bound=_XMLObject) and update _UpdateAction to inherit from Generic[T] and use objects: list[T].
  • Update Create, Delete, and Change to specify their allowed unions of types via the type parameter, matching the registry and KML spec.
  • If needed, use TYPE_CHECKING imports to avoid runtime cycles while still giving mypy (and other checkers) precise types.
  • Run mypy and ensure the new typing doesn’t break existing call sites or public APIs.

Acceptance criteria

  • Change can be used (and round-tripped) with all KML object types that AbstractObjectGroup allows, not just features.
  • Type checkers enforce that each of Create, Delete, and Change only contain the KML object types permitted by the spec.
  • Existing behavior of Update/NetworkLinkControl is preserved for valid inputs.

cleder avatar Dec 03 '25 11:12 cleder