fix: Clean up awkward user-facing tune/config output lines - #27
Conversation
- Remove stray spaces in ESTIMATED tuning header line Fixes vyncint#25 Signed-off-by: Vedant Madane <6527493+VedantMadane@users.noreply.github.com>
Signed-off-by: Vedant Madane <6527493+VedantMadane@users.noreply.github.com>
vyncint
left a comment
vyncint
left a comment
There was a problem hiding this comment.
Thanks for picking this up — both string changes are correct, and the sweep is complete: there is no user-facing (s) left anywhere in crates/.
Three things before it can land, and the first two are quick.
1. Two stray files. COMMITMSG and PRBODY.md are in the diff at the repo root. They look like local scratch files for composing the commit message and PR body — git rm them and they are gone.
2. The goldens need re-blessing — this is why the four checks are red. app.rs now emits configuration, but overview-80x24.txt and overview-110x32.txt still record configuration(s):
assertion `left == right` failed: overview: frame differs from golden overview-80x24.txt
- │1 REFUSED configuration(s) measured FASTER than the chosen one — view 3 │
+ │1 REFUSED configuration measured FASTER than the chosen one — view 3 │
One command, and I ran it against your branch to be sure of what it does:
LAUNCHBOUND_BLESS=1 cargo test -p launchbound-tui --test tuiIt changes exactly those two lines in those two files, and the suite then passes 6/6. Please read the diff before committing it, the way tests/tui.rs asks — blessing without looking is how a golden ends up recording a bug.
3. Optional, and genuinely not a blocker. The else branch is never exercised: the only fixture (run-flip) produces exactly one rejected-faster candidate, so "configurations" is unreached by any test. If adding a second one to the fixture is easy, it would cover both sides. If it turns out to be fiddly, leave it — I would rather not turn a two-line fix into a fixture project.
On the line-splitting I floated in #25: it was optional and the header reads fine as it is now. No need.
Three things from the review on vyncint#27, none of them the contributor's string changes, which were right. The diff carried COMMITMSG and PRBODY.md at the repository root — local scratch files for composing the commit message and the PR body. Removed. The goldens still recorded `configuration(s)`, so all four checks were red: `overview_at_80x24`, `resize_relayouts_the_frame` and `stress_100_runs_at_80x24`, on both runners, plus the same three inside `ci`. Re-blessed with LAUNCHBOUND_BLESS=1, and the diff is exactly the two banner lines in the two goldens that carry it — nothing else moved, which is the point of reading a bless before committing it. The `else` branch had no test and could not easily get one: the count comes from measured timings, and the single fixture the golden frames are built on yields exactly one rejected-faster candidate. Reaching the plural through a rendered frame would mean rebuilding that fixture and moving every golden in the suite to cover one word. So the banner is a function now, and both arities are asserted directly. That is a better shape for it anyway — a `format!` with an `if` inside it is harder to read than a named thing, and this one is the line the view exists to draw attention to. Rendering is unchanged, which the goldens confirm. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
vyncint
left a comment
vyncint
left a comment
There was a problem hiding this comment.
All three review items are done — I pushed the last commit to your branch since maintainerCanModify was on and the fix was mechanical.
COMMITMSG/PRBODY.mdremoved.- Goldens re-blessed. The diff is exactly the two banner lines, nothing else — worth checking, since a bless that moves more than you expected is how a golden ends up recording a bug.
- The banner is a small function now, with both arities asserted. Reaching the plural through a rendered frame would have meant rebuilding the fixture every golden is built on, to cover one word — not a trade worth making for a two-line fix, and this gets the coverage without it.
All 8 checks green (4 were red). Your string changes were right as they stood; thanks for taking the issue.
2 participants
Summary
Clean up awkward user-facing tune/config output lines
Changes
Fixes #25