Skip to content

Polish release_gate_geotiff.rst for maintainer use (#2345 PR 3, closes #2381)#2388

Merged
brendancol merged 3 commits into
mainfrom
issue-2381
May 25, 2026
Merged

Polish release_gate_geotiff.rst for maintainer use (#2345 PR 3, closes #2381)#2388
brendancol merged 3 commits into
mainfrom
issue-2381

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2381. Part of epic #2345 (PR 3 of the release-gate doc polish wave).

What changed

Out of scope

  • No new regression tests. The acceptance text states "if a gate row
    points at a missing test, file a follow-up issue rather than write
    the test here". I checked every test path referenced in the file
    and all 99 cited tests exist, so no follow-up was needed.
  • geotiff.rst's tier table is untouched per the issue scope.

Test plan

  • pytest xrspatial/geotiff/tests/test_release_gate_2321.py xrspatial/geotiff/tests/test_supported_features_tiers_2137.py passes (24 passed, 1 xpassed).
  • Every test path referenced in the doc exists on disk.
  • Every row tier is one of stable / advanced / experimental /
    internal_only and matches SUPPORTED_FEATURES where the feature
    key appears in that constant.
  • The new "how to run" section names the pytest invocation, the
    skip-handling rule, and the promote / demote decision rule.
  • RST parses cleanly (docutils round-trip; Sphinx build runs in CI).

* Add a "How a maintainer runs this gate" section at the top covering the
  pytest invocation, skip-handling guidance, and the promote / demote
  decision rule.
* Add an Epic column to every list-table so each row links to its owning
  epic (#2286, #2321, #2340, #2341, #2342, #2344). Epic anchors are
  collected at the bottom of the page.
* Fix tier mismatches against SUPPORTED_FEATURES: ``reader.allow_rotated``
  and ``reader.allow_unparseable_crs`` move to ``experimental`` to match
  the runtime constant. Replace freeform tier strings like "rejected at
  writer boundary" with the parent feature's tier where the row is a
  sub-gate of an existing feature.
* Refresh the placeholder PR cross-reference block: drop the stale sub-PR
  list now that the gate sub-PRs are landing, and reframe the parent-issue
  note as an epic reference list at the bottom of the page.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 25, 2026
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

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

PR Review: Polish release_gate_geotiff.rst for maintainer use (#2345 PR 3, closes #2381)

Docs-only PR. Algorithm / backend / performance parts of the review template don't apply. I checked doc accuracy and the acceptance criteria from #2381.

Blockers

None.

Suggestions

  • release_gate_geotiff.rst:23-27: the top note names the six owning epics but skips #2345, which is the epic this PR is part of. Add a line pointing at #2345 so the file traces back to its own parent.
  • release_gate_geotiff.rst:312-315: the reader.http row still says (see ``#2321`` sub-PR 5). The "Placeholder PR cross-references" section that gave that label meaning is gone (good), so the reference now dangles. Replace sub-PR 5 with #2326 or drop the parenthetical.
  • release_gate_geotiff.rst:326-335 and 336-342: the SSRF and per-tile byte-cap rows are stable while their parent rows (reader.http, reader.http_cog) are advanced. The mismatch is intentional but reads as drift. One sentence in "How a maintainer runs this gate" noting that sub-gates can carry a stricter tier than their parent row would head off a future audit flag.
  • geotiff_release_contract.md:93-94 (out of scope for this PR): the companion doc still tiers reader.allow_rotated and reader.allow_unparseable_crs at advanced, which now conflicts with this PR's experimental. File a follow-up so the two files don't drift.

Nits

  • release_gate_geotiff.rst:46-51: the -k release_gate filter assumes every release-gate test file is named test_release_gate_*.py. Worth one ls check; if any cited file does not match, the recommended pytest invocation under-runs the gate.
  • release_gate_geotiff.rst:79-82: the "newly-passing xfail" guidance says re-tier the row but does not say "remove the xfail marker in the same commit". Worth spelling out.
  • release_gate_geotiff.rst:172: row title eager / dask parity (sub-gate of ``reader.dask``) uses a parenthetical, while rows like reader.windowed -- shifted-transform parity use a dash. Pick one form so the row index scans.

What looks good

  • All three required pieces of the "how to run" section are present: pytest invocation, skip handling, promote/demote rule.
  • Every row has all five required pieces (feature, tier, acceptance, test, epic).
  • The two SUPPORTED_FEATURES tier mismatches (reader.allow_rotated, reader.allow_unparseable_crs) are fixed.
  • 99/99 cited test paths exist on disk. No missing-test follow-up needed.

Checklist

  • Doc parses (docutils; local Sphinx env unrelated)
  • Every row has acceptance + test path + epic + tier
  • Every tier value matches SUPPORTED_FEATURES
  • Every cited test exists on disk
  • "How a maintainer runs this gate" covers the three required pieces
  • geotiff.rst tier table untouched
  • No new tests added (scope: file follow-up if needed)
  • [n/a] Algorithm / backend correctness (docs-only)
  • [n/a] README matrix (docs-only)

* Add a reference to the doc-readiness epic (#2345) in the top note so
  the file traces back to its own parent epic, the same way every row
  traces back to its owning epic.
* Replace the dangling "see #2321 sub-PR 5" parenthetical on the
  ``reader.http`` row with a direct ``#2326`` reference. The sub-PR
  numbering vehicle was removed earlier in this PR; the inline
  reference was hanging.
* Add a "note on sub-gate rows" subsection explaining that sub-gate
  rows can carry a stricter tier than their parent feature row
  (the SSRF and per-tile byte-cap rows under ``reader.http`` /
  ``reader.http_cog``). The mismatch is intentional and should not
  be flagged as drift by a gate auditor.
* Spell out the action on a newly-passing xfail: remove the xfail
  marker in the same commit that re-tiers the row, otherwise the
  gate can silently regress.
* Normalize sub-gate row titles to the ``parent`` -- description form
  used elsewhere in the file. Drop the redundant ``(sub-gate)``
  parenthetical now that the convention has its own section.

Follow-up #2389 filed for the parallel tier drift in
``docs/source/reference/geotiff_release_contract.md`` (out of scope
for this PR per the #2381 acceptance).
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

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

PR Review (round 2): Polish release_gate_geotiff.rst (#2345 PR 3, closes #2381)

Re-reviewed after ac8e979c. Every actionable item from round 1 was addressed in-PR. Round 1 deferral (geotiff_release_contract.md tier drift) is tracked in #2389.

Blockers

None.

Suggestions

None.

Nits

  • release_gate_geotiff.rst:328: the new #2326 reference resolves to a merged PR rather than an issue. The other inline #... references in the file all point at issues. Not a bug (the link works), but if you want full consistency, swap for #2323 (the parent issue that #2326 closed) and leave the prose unchanged. Optional; ignore if you'd rather keep the direct PR pointer.

What looks good

  • The "doc-readiness epic that owns this checklist itself is #2345" line at line 27-28 closes the round-1 trace-back gap.
  • The new "A note on sub-gate rows" subsection explains the intentional tier-mismatch (SSRF row stable while parent reader.http is advanced) cleanly. Future audit passes will not file this as drift.
  • Row title reader.dask -- eager / dask parity now matches the dash convention used elsewhere. The redundant (sub-gate) parentheticals are gone.
  • The "newly-passing xfail" guidance now says exactly what to do: remove the marker in the same commit.

Checklist

  • Round-1 blockers / suggestions / nits addressed in-PR (or deferred to filed follow-up)
  • Every gate row still has acceptance + test path + epic + tier
  • Every tier value still matches SUPPORTED_FEATURES
  • Every cited test still exists on disk
  • RST parses cleanly (docutils round-trip)

Round-2 review nit: every other inline ``#NNNN`` reference in the file
points at an issue, not a PR. Swap the case-insensitive scheme routing
reference from #2326 (the merged PR) to #2323 (the parent issue that
PR closed) so the convention is uniform.
@brendancol brendancol merged commit 9887d72 into main May 25, 2026
4 of 7 checks passed
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.

Release checklist polish in release_gate_geotiff.rst (#2345 PR 3)

1 participant