geotiff: rename write_vrt's vrt_path kwarg to path (#1946) (#1962)

* geotiff: rename write_vrt's vrt_path kwarg to path (#1946)

write_vrt's first positional kwarg was vrt_path while to_geotiff and
write_geotiff_gpu use path, so callers passing write_vrt(path=...) hit
TypeError on the only writer whose docstring advertises parity with the
other two. Adds path as the canonical kwarg, keeps vrt_path as a
deprecated alias (DeprecationWarning), and raises TypeError if both
are supplied. Mirrors the existing crs / crs_wkt shim in the same
writer (#1715).

Tests pin positional and keyword forms, the deprecation warning on
vrt_path, the both-supplied refusal, the cross-writer signature
parity, and a round-trip equivalence between the two kwarg names.

* geotiff: address PR #1962 review / CI fixes

* geotiff: address PR #1962 review (sentinel for write_vrt path, docstring TypeError note)

Switch write_vrt's path kwarg from a None default to a private
_VRT_PATH_MISSING_SENTINEL so an explicit write_vrt(path=None, ...)
or positional write_vrt(None, sources) is rejected with a clear
TypeError naming the offending kwarg, instead of falling through
to the "missing required argument" branch. Mirrors the existing
_CRS_WKT_DEPRECATED_SENTINEL pattern in the same writer.

Also mirror the crs_wkt entry's "passing both raises TypeError"
wording in the write_vrt entry of the geotiff Public API docstring.

Updates the test_write_vrt_path_annotated annotation assertion
from 'str | None' to 'str' to match the new sentinel-typed default
and adds two new regression tests pinning the keyword and
positional explicit-None cases.

Author: brendancol

xrspatial/geotiff/__init__.py

@@ -26,8 +26,10 @@
 write_geotiff_gpu(data, path, ...)
     GPU-only writer using nvCOMP. ``to_geotiff(..., gpu=True)`` calls this
     internally.
-write_vrt(vrt_path, source_files, ...)
-    Generate a VRT mosaic XML from a list of GeoTIFF files.
+write_vrt(path, source_files, ...)
+    Generate a VRT mosaic XML from a list of GeoTIFF files. ``vrt_path``
+    is kept as a deprecated alias for ``path``; passing both ``path`` and
+    ``vrt_path`` raises ``TypeError`` (#1946).
 """
 from __future__ import annotations
 

xrspatial/geotiff/_runtime.py

@@ -39,6 +39,21 @@
 # (forward verbatim to read_vrt). Mirrors the on_gpu_failure pattern. See
 # issue #1810.
 _MISSING_SOURCES_SENTINEL = object()
+# ``write_vrt`` historically named its first positional kwarg ``vrt_path``
+# while ``to_geotiff`` / ``write_geotiff_gpu`` use ``path``. The deprecation
+# shim adds ``path`` as the new name and accepts ``vrt_path`` with a
+# DeprecationWarning. The sentinel pattern distinguishes "user passed
+# vrt_path= explicitly" from "user passed nothing", which is the same
+# rationale ``_CRS_WKT_DEPRECATED_SENTINEL`` documents above. See
+# issue #1946.
+_VRT_PATH_DEPRECATED_SENTINEL = object()
+# ``write_vrt`` also needs to distinguish "user passed path= explicitly"
+# (including an explicit ``path=None``, which is an error) from "user
+# passed nothing" (fall through to the ``vrt_path`` shim). Without this
+# sentinel, ``write_vrt(None, sources)`` silently fell through to the
+# ``path is None`` branch and raised a "missing required argument"
+# TypeError for the wrong reason. See PR #1962 review.
+_VRT_PATH_MISSING_SENTINEL = object()
 
 
 # Spatial dim names recognised on 3D writer inputs. ``y``/``x`` are the

xrspatial/geotiff/_writers/vrt.py

@@ -1,19 +1,25 @@
 """VRT writer entry point.
 
 Wraps ``_vrt.write_vrt`` with the public ``write_vrt`` surface:
-deprecation handling for ``crs_wkt`` (#1715), normalisation of the
-``crs`` kwarg to WKT via ``_resolve_crs_to_wkt``, and the parity
-docstring vs ``to_geotiff`` / ``write_geotiff_gpu``.
+deprecation handling for ``crs_wkt`` (#1715) and ``vrt_path`` (#1946),
+normalisation of the ``crs`` kwarg to WKT via ``_resolve_crs_to_wkt``,
+and the parity docstring vs ``to_geotiff`` / ``write_geotiff_gpu``.
 """
 from __future__ import annotations
 
 import warnings
 
 from .._crs import _resolve_crs_to_wkt
-from .._runtime import _CRS_WKT_DEPRECATED_SENTINEL
+from .._runtime import (
+    _CRS_WKT_DEPRECATED_SENTINEL,
+    _VRT_PATH_DEPRECATED_SENTINEL,
+    _VRT_PATH_MISSING_SENTINEL,
+)
 
 
-def write_vrt(vrt_path: str, source_files: list[str], *,
+def write_vrt(path: str = _VRT_PATH_MISSING_SENTINEL,
+              source_files: list[str] | None = None, *,
+              vrt_path: str | None = _VRT_PATH_DEPRECATED_SENTINEL,
               relative: bool = True,
               crs: int | str | None = None,
               crs_wkt: str | None = _CRS_WKT_DEPRECATED_SENTINEL,
@@ -22,10 +28,19 @@ def write_vrt(vrt_path: str, source_files: list[str], *,
 
     Parameters
     ----------
-    vrt_path : str
-        Output .vrt file path.
+    path : str
+        Output .vrt file path. Mirrors the ``path`` kwarg on
+        ``to_geotiff`` and ``write_geotiff_gpu`` so the writer trio
+        shares a single destination-arg name (issue #1946).
     source_files : list of str
         Paths to the source GeoTIFF files.
+    vrt_path : str, optional
+        Deprecated alias for ``path``. Emits ``DeprecationWarning`` when
+        supplied; passing both ``path`` and ``vrt_path`` raises
+        ``TypeError``. Kept so existing callers (``write_vrt(vrt_path,
+        sources)`` positional or ``write_vrt(vrt_path=...)`` keyword)
+        keep working through the deprecation window. New code should
+        use ``path``. See issue #1946.
     relative : bool, optional
         Store source paths relative to the VRT file (default True).
     crs : int, str, or None, optional
@@ -60,6 +75,53 @@ def write_vrt(vrt_path: str, source_files: list[str], *,
     # historic ``crs_wkt`` path; the new ``crs`` path normalises through
     # ``_resolve_crs_to_wkt`` before forwarding because the internal
     # writer still only speaks WKT.
+    #
+    # The ``path`` / ``vrt_path`` shim resolves the destination kwarg
+    # before any other processing so the rest of the function works
+    # uniformly against a single ``vrt_path`` local. ``path`` is the
+    # new name (parity with to_geotiff / write_geotiff_gpu); ``vrt_path``
+    # is kept as a deprecated alias to preserve back-compat for callers
+    # using either positional ``write_vrt(vrt_path, sources)`` or
+    # keyword ``write_vrt(vrt_path=...)``.
+    path_passed = path is not _VRT_PATH_MISSING_SENTINEL
+    vrt_path_passed = vrt_path is not _VRT_PATH_DEPRECATED_SENTINEL
+    if path_passed and vrt_path_passed:
+        # Both supplied is ambiguous regardless of whether the two values
+        # happen to be the same string. Refuse rather than silently
+        # picking one. Mirrors the same rule the ``crs`` / ``crs_wkt``
+        # shim below applies.
+        raise TypeError(
+            "write_vrt: pass either 'path' or the deprecated 'vrt_path' "
+            "alias, not both.")
+    if vrt_path_passed:
+        warnings.warn(
+            "write_vrt(..., vrt_path=...) is deprecated; use path=... "
+            "instead. The kwarg was renamed for parity with to_geotiff "
+            "and write_geotiff_gpu, which already accept 'path' as the "
+            "destination kwarg.",
+            DeprecationWarning,
+            stacklevel=2,
+        )
+        path = vrt_path
+    elif not path_passed:
+        # Neither name supplied. Match the previous ``TypeError: missing
+        # required positional argument`` semantics by raising rather than
+        # forwarding the sentinel into ``_write_vrt_internal``.
+        raise TypeError(
+            "write_vrt: missing required argument 'path'")
+    if path is None:
+        # Explicit ``path=None`` (including positional ``write_vrt(None,
+        # sources)``) is rejected up front so the error message names the
+        # offending kwarg instead of crashing deep in
+        # ``os.path.dirname(os.path.abspath(None))``. The sentinel default
+        # on ``path`` is what lets us distinguish this case from "caller
+        # passed nothing" above.
+        raise TypeError(
+            "write_vrt: 'path' must be a str, got None")
+    if source_files is None:
+        raise TypeError(
+            "write_vrt: missing required argument 'source_files'")
+
     crs_wkt_passed = crs_wkt is not _CRS_WKT_DEPRECATED_SENTINEL
     if crs is not None and crs_wkt_passed:
         # Both supplied is ambiguous regardless of whether the WKT happens
@@ -83,7 +145,7 @@ def write_vrt(vrt_path: str, source_files: list[str], *,
 
     from .._vrt import write_vrt as _write_vrt_internal
     return _write_vrt_internal(
-        vrt_path, source_files,
+        path, source_files,
         relative=relative,
         crs_wkt=resolved_wkt,
         nodata=nodata,

xrspatial/geotiff/tests/test_signature_annotations_1654.py

@@ -83,10 +83,24 @@ def test_write_geotiff_gpu_path_annotated():
     assert 'BinaryIO' in ann
 
 
+def test_write_vrt_path_annotated():
+    """``write_vrt(path, ...)`` is str-only (VRT writes are path-only by
+    design; no file-like buffer support). After #1946 the canonical name
+    is ``path`` (parity with ``to_geotiff`` / ``write_geotiff_gpu``).
+    The annotation is plain ``str``: the default value is a private
+    sentinel (not ``None``) so the deprecation shim can distinguish
+    ``write_vrt(path=None, ...)`` (rejected with TypeError) from a
+    caller who omitted ``path`` entirely (routed through the ``vrt_path``
+    alias). See PR #1962 review."""
+    assert _annotation(write_vrt, 'path') == 'str'
+
+
 def test_write_vrt_vrt_path_annotated():
-    """``write_vrt(vrt_path, ...)`` stays str-only (VRT writes are
-    path-only by design; no file-like buffer support)."""
-    assert _annotation(write_vrt, 'vrt_path') == 'str'
+    """The deprecated ``vrt_path`` alias keeps the same ``str | None``
+    annotation as ``path`` (str-only at the type level; ``None`` only
+    appears because the sentinel default lets the shim detect omission).
+    Pinned so a future re-rename does not silently widen the alias."""
+    assert _annotation(write_vrt, 'vrt_path') == 'str | None'
 
 
 # --- source: str or BinaryIO (open_geotiff is the public dispatch) ---