Skip to content

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

Open
brendancol wants to merge 3 commits into
mainfrom
fix-write-vrt-path-kwarg-1946
Open

geotiff: rename write_vrt's vrt_path kwarg to path (#1946)#1962
brendancol wants to merge 3 commits into
mainfrom
fix-write-vrt-path-kwarg-1946

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #1946.

Summary

  • write_vrt now accepts path as its destination kwarg, matching to_geotiff and write_geotiff_gpu.
  • vrt_path is kept as a deprecated alias that emits DeprecationWarning; passing both path and vrt_path raises TypeError.
  • Mirrors the existing crs / crs_wkt shim in the same writer (geotiff: write_vrt's crs_wkt kwarg drifts from to_geotiff/write_geotiff_gpu's crs #1715) and uses the same sentinel pattern (_VRT_PATH_DEPRECATED_SENTINEL in _runtime.py).

Why

xrspatial.geotiff exposes three writers that the docstring advertises as parity siblings. to_geotiff and write_geotiff_gpu use path; write_vrt was using vrt_path, so write_vrt(path=...) failed with TypeError: unexpected keyword argument despite the writer trio claiming a shared surface. Detected by the deep-sweep-api-consistency sweep.

Test plan

  • pytest xrspatial/geotiff/tests/test_write_vrt_path_kwarg_1946.py — 8 new tests pass locally
    • positional write_vrt(path, sources) works without warning
    • keyword write_vrt(path=..., source_files=...) works without warning
    • keyword write_vrt(vrt_path=...) works and emits DeprecationWarning
    • passing both raises TypeError
    • omitting both raises TypeError
    • inspect.signature pins path as the first positional arg
    • cross-writer signature parity (to_geotiff/write_geotiff_gpu/write_vrt all use path)
    • round-trip equivalence between the two kwarg names
  • CI green

Copilot AI review requested due to automatic review settings May 15, 2026 17:04
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 15, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

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.
@brendancol brendancol force-pushed the fix-write-vrt-path-kwarg-1946 branch from 4d3061f to 6e46c58 Compare May 15, 2026 17:54
@brendancol
Copy link
Copy Markdown
Contributor Author

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

Blockers (must fix before merge)

  • None

Suggestions (should fix, not blocking)

  • xrspatial/geotiff/_writers/vrt.py:19-25 — the path kwarg signature is path: str | None = None with a real None default, while every other kwarg in the writer trio that uses the sentinel pattern uses a private sentinel object so the shim can tell "user passed None" from "user passed nothing". With the current shape, write_vrt(None, sources) (positional None) silently falls through to the path is None branch and raises TypeError: missing required argument 'path', which is the right behaviour but for the wrong reason. Consider using a _PATH_SENTINEL and treating an explicit path=None as the ambiguous case the way crs_wkt does, or document the asymmetry. Not load-bearing but worth a comment for the next reader.
  • xrspatial/geotiff/__init__.py:29-31 — the Public API docstring entry for write_vrt mentions vrt_path is "kept as a deprecated alias for path" but does not call out that the shim raises TypeError when both are passed. The crs_wkt entry (further up the same docstring family in _writers/vrt.py:51-58) does spell that rule out; mirror the wording here for consistency.

Nits (optional improvements)

  • xrspatial/geotiff/_writers/vrt.py:80-85 — the long comment block explaining the shim is good but the phrase "preserve back-compat" reads slightly informal next to the rest of the file. Minor.
  • xrspatial/geotiff/tests/test_write_vrt_path_kwarg_1946.py:151-170test_write_vrt_first_arg_name_matches_writer_trio duplicates the assertion in test_write_vrt_signature_first_arg_is_path for write_vrt. The trio-parity check could just call the per-function tests, but keeping it standalone is fine if the intent is to catch any of the three writers drifting in isolation.

What looks good

  • The deprecation shim mirrors the existing crs_wkt (geotiff: write_vrt's crs_wkt kwarg drifts from to_geotiff/write_geotiff_gpu's crs #1715) shape in the same file, which keeps the writer self-consistent.
  • Both-supplied rejection is tested with pytest.raises(TypeError, match="path.*vrt_path"), and the deprecation warning is tested with pytest.warns(DeprecationWarning, match='vrt_path'). Round-trip equivalence between old and new kwarg is also pinned.
  • Signature parity is asserted against to_geotiff and write_geotiff_gpu so a future re-rename trips a test.
  • The sentinel lives in _runtime.py alongside the other writer-trio sentinels, preserving the module-cache identity rule the file documents.

CI status

  • macOS 3.13 (and the rest of the matrix) failed on xrspatial/geotiff/tests/test_signature_annotations_1654.py::test_write_vrt_vrt_path_annotated: PR-induced. The pre-existing test pinned vrt_path's annotation to 'str', but the deprecation default of a sentinel forces the annotation to widen to 'str | None'. Fixed in 6e46c58 by updating that test and adding a sibling test_write_vrt_path_annotated for the new canonical name.
  • request-copilot-review: passed (no review content posted).
  • ReadTheDocs: passed.

Backward compatibility

  • The old vrt_path= kwarg is still accepted and emits DeprecationWarning. Positional write_vrt(some_path, sources) keeps working unchanged because path sits where vrt_path used to. Passing both raises TypeError. This is the right call for an API rename in this project — the same shape crs_wkt/crs shipped under (geotiff: write_vrt's crs_wkt kwarg drifts from to_geotiff/write_geotiff_gpu's crs #1715).
  • A repo-wide grep for write_vrt(vrt_path= finds no call sites in the codebase that still use the deprecated alias. Other matches (xrspatial/geotiff/_writers/eager.py, several test files) are local variable names that happen to be called vrt_path and are passed positionally — those keep working.

Checklist

  • Algorithm matches reference/paper (N/A — kwarg rename only)
  • All implemented backends produce consistent results (no backend code touched)
  • NaN handling is correct (no data-path changes)
  • Edge cases are covered by tests (both-supplied, neither-supplied, positional, keyword, deprecation, round-trip)
  • Dask chunk boundaries handled correctly (N/A)
  • No premature materialization or unnecessary copies (N/A)
  • Benchmark exists or is not needed (not needed for a signature change)
  • README feature matrix updated (if applicable) — N/A
  • Docstrings present and accurate

…ing 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.
@brendancol
Copy link
Copy Markdown
Contributor Author

Addressed in 9f83e12.

  • _writers/vrt.py: path now defaults to a new _VRT_PATH_MISSING_SENTINEL (alongside the existing _VRT_PATH_DEPRECATED_SENTINEL for vrt_path). Explicit path=None or positional write_vrt(None, sources) raises TypeError with a message that names 'path'. Caller-omitted falls through to the vrt_path shim as before.
  • __init__.py Public API docstring: the write_vrt entry now spells out that passing both path and vrt_path raises TypeError, mirroring the crs_wkt rule.
  • Tests: signature annotation for path is now plain str (test updated). Added two cases for explicit-None (keyword and positional).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance PR touches performance-sensitive code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

geotiff: write_vrt's first positional kwarg vrt_path drifts from to_geotiff/write_geotiff_gpu path

2 participants