Skip to content

fix: reject out-of-bounds row text access - #268

Merged
vyncint merged 1 commit into
vyncint:mainfrom
nightcityblade:fix/issue-260
Sep 5, 2026
Merged

fix: reject out-of-bounds row text access#268
vyncint merged 1 commit into
vyncint:mainfrom
nightcityblade:fix/issue-260

Conversation

@nightcityblade

Copy link
Copy Markdown
Contributor

What & why

Make Screen::row_text panic with a descriptive message for an out-of-bounds row, preventing invalid coordinates from looking like a legitimately blank row. The docs now distinguish this behavior from cell and clamped rect_text ranges, and a focused regression test covers it.

Closes #260.

Checklist

  • Linked an issue (or explained above why none exists)
  • Tests added/updated for the change
  • cargo fmt --all and cargo clippy --workspace --all-targets --all-features are clean
  • All commits are signed off (git commit -s) — see CONTRIBUTING.md §5
  • No AI attribution trailers (no AI co-authors, "Generated with" footers, or bot identities) — see CONTRIBUTING.md §6
  • CHANGELOG.md updated under [Unreleased] (user-facing changes only)
  • Snapshot changes (if any) were reviewed with cargo insta review, not blind-accepted (N/A: no snapshots changed)

@vyncint vyncint left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Reviewed and verified. The implementation is right.

What I checked beyond CI:

  • The panic boundary is exact: on a 20x4 screen row_text(3) returns its row and row_text(4) panics with row_text: row 4 is outside the 4-row screen. The message follows rect_text's shape, which is what makes the two read as one decision.
  • The only internal caller is text(), which iterates 0..self.rows, so nothing inside the crate can trip the new assertion. Full suite green, both profiles.
  • The expect replacing the let … else is unreachable by construction once the assertion has run, and it keeps the branch that was already there rather than adding one.

The # Panics section reads well, and pointing at cell for the case where None is the wanted answer is the right cross-reference.

One more change, alongside the shared one below

The entry belongs under Changed, not Fixed. The CHANGELOG preamble states the rule:

Until 1.0, minor versions (0.x) may contain breaking changes; they are always listed under a Changed or Removed heading.

A call that used to return "" and now panics is a behaviour a user must relearn, so it needs to be where someone scanning for breakage will look. Worth adding the one-line migration too, in the shape the neighbouring entries use: a caller who was reading a row index that may be out of range should bounds-check against rows() first, or use cell(row, 0) and treat None as absent.


One thing to fix, and it is the same in all five of your PRs

The CHANGELOG entry landed inside the published ## [0.9.0] - 2026-09-05 section rather than under ## [Unreleased]. Easy to miss: [Unreleased] was empty when you branched, so the nearest heading with content under it belongs to 0.9.0.

It matters for two reasons. 0.9.0 is already on crates.io and its GitHub Release notes were generated from that section, so the entry now describes a release that does not contain the change. And docs/RELEASING.md cuts the next release by moving [Unreleased] down into a new version heading — an entry parked in [0.9.0] would never appear in any release notes at all.

Move it up under ## [Unreleased], adding the ### Added / ### Changed / ### Fixed sub-heading there if it does not exist yet. main has moved on by a commit since you branched, so a rebase is wanted anyway:

git fetch origin && git rebase origin/main

Thanks for these — five well-scoped fixes with tests and docs, and the PR bodies say what you decided and why. That is exactly the shape this repository asks for.

@nightcityblade

Copy link
Copy Markdown
Contributor Author

Rebased onto current main and updated the changelog in 30702e2: the entry now sits under UnreleasedChanged and includes the requested migration guidance.

Validation: cargo fmt --check and cargo test --workspace --all-features pass.

Signed-off-by: nightcityblade <nightcityblade@gmail.com>

@vyncint vyncint left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks — both points addressed: the entry moved to [Unreleased] under Changed, and the migration line tells a caller with a possibly-invalid index what to do instead.

Re-verified on this head: the panic boundary is exact (row_text(3) returns its row on a 4-row screen, row_text(4) panics with the message naming both numbers), text() is unaffected because it iterates 0..rows, and the full suite is green.

I rebased the branch onto main myself and resolved the changelog collision with #267, which landed a few minutes ago and claimed the same lines. Your commit, author and sign-off are unchanged; the only difference is that [Unreleased] now carries Added, Changed and Fixed in Keep a Changelog order. Nothing for you to do. Merging.

@vyncint
vyncint merged commit 7d8a108 into vyncint:main Sep 5, 2026
13 checks passed
vyncint added a commit that referenced this pull request Sep 5, 2026
)

The entries from #268, #269, #270 and #271 were dropped while resolving the
changelog collisions those four PRs had with one another. The script used to
rebuild each conflict region looked for `### Heading` blocks inside it and
wrote nothing when the conflict fell within a section instead of around its
heading — and nothing checked that its output was non-empty, so it failed
silently. Restored verbatim as their authors wrote them; no code changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

row_text answers an out-of-range row with "", the quiet answer rect_text refuses to give

2 participants