Skip to content

geotiff: converge the two deprecated-attr emit helpers in _attrs.py #2017

@brendancol

Description

@brendancol

Background

PR 7 of issue #1984 deprecated 13 attrs across four slices:

xrspatial/geotiff/_attrs.py now exposes two emit helpers that do almost the same job:

def _emit_deprecated_geokey_attr(attrs, name, value, *, reason): ...

def _emit_deprecated_attr(attrs, name, value, *, reason, migration=None): ...

_emit_deprecated_geokey_attr covers the three GeoKey slices and bakes the sentence "...{reason} so it will not round-trip..." into the warning text. _emit_deprecated_attr covers the colormap variants and supports an optional migration= clause (e.g., "Reshape attrs['colormap'] to (n_colors, 3) and append an alpha channel").

The split exists because the warning-text contracts are pinned by separate test suites:

  • test_attrs_pr7_deprecate_geographic_1984.py asserts on the GeoKey-tier wording verbatim via _deprecated_geographic_geokey_warning.
  • test_attrs_pr7_deprecate_vertical_1984.py and test_attrs_pr7_deprecate_projected_1984.py assert on substring matches.
  • test_attrs_pr7_deprecate_colormap_variants_1984.py asserts on the byte-identical pre-refactor wording for cmap and colormap_rgba.

Unifying them was out of scope for the colormap slice but is mechanical.

Proposal

Collapse the two helpers into one:

def _emit_deprecated_attr(
    attrs, name, value, *,
    reason: str,
    migration: str | None = None,
    suffix: str | None = None,
) -> None: ...

Where:

  • reason is the per-tier short clause (e.g., "the writer cannot reconstruct it from the canonical CRS").
  • suffix defaults to "so it will not round-trip." to match the GeoKey-tier baked wording, and is overridden by the colormap slice with its own phrasing.
  • migration is the optional per-attr migration recipe used today only by the colormap variants.

Alternative: drop suffix entirely and require every caller to bake its own "round-trip" sentence into reason. That makes the helper simpler but forces a coordinated update of the three GeoKey test suites that pin the current wording.

Acceptance criteria

  • One emit helper in _attrs.py. The text builder _deprecated_geokey_warning either stays as a thin alias for the GeoKey-tier callers or gets folded in.
  • All four PR 7 test suites (test_attrs_pr7_deprecate_*_1984.py) pass without their existing assertions weakening. If the wording must shift to consolidate, update the assertions in the same PR.
  • _stacklevel_to_external_caller() keeps doing the heavy lifting for stacklevel resolution; no behavior change there.
  • _DEPRECATED_GEOGRAPHIC_GEOKEY_ATTRS stays as the tier-membership constant; mirror constants for the projected, vertical, and colormap-variants tiers if it makes the tier tests cleaner.

Why this is bounded

The two helpers were left side-by-side because the colormap slice landed last and unifying them required touching three sibling test suites. With all four slices now in main, the work is a single PR that touches one file plus the four test files. No production-behavior change visible to users.

Refs #1984.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions