refactor(output): one column renderer, one kind word, one status text - #264
Merged
Merged
Conversation
Plain text and JSON are the whole user-visible surface of internal/output, and both are assembled from four list printers, a detail block and a Markdown brief that repeat each other's measuring and padding. Before folding that repetition into shared helpers, capture what it currently produces. TestRenderGolden renders a fixed fixture through Print for tasks, projects, areas and tags, through PrintTaskList with and without a view label, through PrintTaskWithChecklist for a to-do, a project and a repeating item, through PrintHint, and through PrintAgentBrief for a task, an open project, an empty project, a closed project, a repeating item and a note carrying its own code fence. Each case renders at both ends of the colour profile, and the task listings render at three terminal widths so the column dropping is pinned too. The result is compared against a committed file. The golden values were captured from unmodified rendering code. The one production change here is the seam the test needs: termWidth becomes a var, like nowFn beside it, so a test can pin a width instead of depending on how the test binary's stdout happens to be attached. The body is unchanged. Every line in the golden file ends in "|" because a padded column emits trailing spaces an editor or a hook would otherwise eat, and ANSI escapes are written "\e" so the file stays readable.
printTasks, printProjects, printAreas and printTags each declared a local row struct, measured every column with lipgloss.Width and padded with padCol before joining with a gap — four copies of the same twenty lines, and the copy inside printTasks also carried the terminal-width column dropping. There is now one `table` in table.go: rows of cells go in, measured and padded lines come out, and the column dropping is a `dropOrder` field rather than a pair of booleans. The rule the four copies shared is written down once: every column is padded to its widest cell except the last one declared, which goes out as it is. Dropping a column does not move that rule, which is why a narrow listing still pads its title column — the behaviour the copies had, now stated rather than implied. printTasks keeps its group-header logic, which no table can own: it needs to write a header between two rows. `table.lines()` hands back one line per row for exactly that, so the header fold from #205 is untouched. `t.Type == model.TypeProject` was branched on six times across the two files. It is now `isProject(t)`, and the word that goes in the output is `kindWord(t)` — which takes "task" and "project" from model's type codec, the same source the JSON `type` field renders from. It is deliberately not TaskType.String(): a heading, or a code Things has yet to write, reads as a task on these surfaces, which is what the CLI has always shown. statusText no longer keeps its own switch over the three statuses. It capitalises model.Status.String(), so the words live in one place and the detail block, the JSON and the agent brief cannot drift apart. checklistLine takes its "(cancelled)" from the same source. statusIcon stays as it is: "[ ]", "[~]" and "[x]" are glyphs, not the status words, and Markdown's two-state checkbox in a brief is a third thing again. No output byte moves, no exported signature changes and no JSON changes: - TestRenderGolden passes untouched. testdata/render_golden.txt does not appear in this commit's diff, which is the claim and the evidence for it. - Against the live database, 226 invocations of the built binary are byte-identical to origin/main, 1.8 MB of output in all: nine views plain, with --include-completed, as JSON, with --color always and with --no-hints; projects, areas and tags in the same three forms; and 43 real items through `show`, `show --agent`, `show --json` and `show --color always`. output.go goes from 395 lines to 337 and agent.go stays where it was; table.go is 150, most of it the doc comments the four copies never had.
TestRenderGolden returned early when the document matched and otherwise reported from inside its per-case loop. A difference the case splitter does not see — trailing whitespace, or text before the first header, both of which it drops — left the loop with nothing to report, and the test passed with got != want. Appending two newlines to the golden file reproduced it. Found by /code-review.
This was referenced Sep 10, 2026
ryanlewis
added a commit
that referenced
this pull request
Sep 10, 2026
Closes #244 ## What changed `internal/db/tasks_test.go` had fifteen seed helpers, one per PR this week, each re-inserting its own areas, projects and to-dos, and the shared assertion helpers `uuidsOf`, `sameSet` and `keysOf` sat beside them rather than in `testhelpers_test.go`. The three assertion helpers move to `testhelpers_test.go`, next to a small fixture builder, and the fourteen per-view seeds are folded into the tests that used them. A test now seeds only the rows it asserts on: `TestListTasksSomedayHidesChildOfSomedayProject` went from nine rows to two, `TestListTasksTodayIncludeCompletedProject` from twelve to three. `seedTasks` stays. Twenty-two tests read it as the general fixture covering every view and status, so folding it would rewrite most of the file for no gain; it is now built through the same builder as everything else. ## The builder ```go d, fx := newFixture(t) fx.area("area-work", "Work", 1) fx.project("proj-today", "Runbook audit", 5, anytimeOn(today), inArea("area-work"), todayIndex(2005)) fx.todo("todo-loose", "Buy milk", 14, anytimeOn(today), todayIndexRef(today), todayIndex(7)) fx.heading("head-1", "Phase one", 11, inProject("proj-today")) fx.tag("tg-urgent", "urgent", 1) fx.tagged("t-today", "tg-urgent") ``` `"index"` is a required argument on every row and every date is passed in. Nothing an assertion can turn on is defaulted, because eleven tests assert order rather than membership and several seed their indexes deliberately against the order they expect, which is what leaves the key under test as the only thing that can produce it. Area and project indexes stay negative, as Things writes them — the ordering CASE keys exist because an unfiled row's `COALESCE` default of 0 would sort it last instead of first (#217, #237). Options name a Things bucket rather than the raw columns: `inbox()`, `anytime()`, `anytimeOn(date)`, `evening(date)`, `someday()`, `somedayOn(date)`. The rest are one per column: `inArea`, `inProject`, `underHeading`, `notes`, `trashed`, `completed(stop)`, `cancelled(stop)`, `status`, `deadline`, `todayIndex`, `todayIndexRef`, `repeats`. ## Also in this pass `TestListTasksTodayIncludeCompletedProject` stopped a row a minute in the past. The calendar day decides whether a closed row is still under Today (#230), so a minute before midnight falls on the previous day and the test failed in the first minute of every day. It now stops the row at `time.Now()`, the convention the rest of the file follows. `TestListQueryGoldenSQL` reported differences only from inside its per-case loop, so a difference outside a `### view=` block — trailing whitespace, text before the first header — passed with `got != want`. This is the two-line fix #264 applied to `TestRenderGolden`. Verified by appending a blank line to the golden file: the test now fails with `generated SQL differs from testdata/list_query_golden.txt outside any case body`. The comment above `TestPrintTasksNoRepeatedProjectHeader` said today, upcoming and anytime all list a scheduled project as a row. Anytime has carried no project rows since #217. ## Evidence The passing test list is identical before and after — 263 tests across `internal/db` and `internal/output`, no test renamed, added or removed: ``` go test ./internal/db ./internal/output -json \ | jq -r 'select(.Action=="pass" and .Test) | .Test' | sort diff before.txt after.txt # no output ``` Per-function coverage is identical too, function for function across both packages, so the folded fixtures still reach every line the fuller ones did: ``` diff <(go tool cover -func=before.cov) <(go tool cover -func=after.cov) # no output; total 95.9% either way ``` Every seed was first rewritten through the builder and pinned row for row against a copy of the SQL it replaced — all four tables dumped and compared column by column, with `stopDate` values bucketed to the minute so two `time.Now()` calls a microsecond apart compare equal. All fifteen matched exactly, including the `todayIndex = 0` values that would sort differently from NULL under the today and upcoming orderings. Only then were rows dropped from a fixture, so the fold below removes rows and never changes a value. That scaffolding is not part of the PR. A differential mutation study confirms the smaller fixtures still bite. Nine predicates in `internal/db/tasks.go` were broken one at a time — `untrashedRows`, `notHeading`, `todoOrProject`, the template-row and template-child exclusions, `untrashedParent`, `unparented`, `todoOnly` and `openRows` — and each mutant run against both the old and the new test file. Every mutation is still caught. Four more, on the view table itself, are caught by the folded tests too: someday losing `unparented`, anytime gaining `includesProjects`, trash losing it, and `todoOrProject` admitting headings. What the fold does cost is redundancy. Five of those nine mutations used to fail `TestListTasksDeadlinesIncludeProjects` and `TestListTasksDeadlinesOrderWithProject` as well, because the shared seed handed them six rows they never asserted on and their exact-set assertions failed on any leak. Each of those five is still caught by `TestListTasksDeadlinesProjectExclusions`, which is the test that owns those rows. The same trade, smaller, applies to `TestListTasksLogbookIncludesCancelled`, `TestListTasksTodayOrderWithProjects` and `TestListTasksViewsIncludeProjects`. Putting the rows back is the duplication this PR set out to remove. `testdata/list_query_golden.txt` regenerates with no diff. ## Notes for review The three arrangement tests that seed negative area and project indexes — `TestAnytimeGroupsByAreaThenProject`, `TestTodayGroupsLooseTodosBeforeProjectTodos` and `TestSomedayGroupsUnfiledThenAreas` — keep their own inline SQL. They were never seed helpers, so converting them is outside what #244 asks for, and their literal indexes are the whole point of those tests. `TestListTasksTrashProjectExclusions` asserts that `trash-head` is not listed, but the row is hidden by the trashed-parent fold rather than by the heading exclusion — it stayed out even with `todoOrProject` mutated to admit type 2. That is unchanged from before this PR; the fixture had the same shape. Its `live-proj` and `live-todo` assertions do bite. `TestListTasksDeadlinesIncludeProjects` and `TestListTasksDeadlinesOrderWithProject` now visibly assert the same listing, one with `sameSet` and one with `reflect.DeepEqual`, and `TestListTasksLogbookIncludesCancelled` and `TestListTasksLogbookCancelledOrder` are the same pair. Both predate this PR and are left alone. No production code changed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #243
What changed
printTasks,printProjects,printAreasandprintTagseach declared a local row struct, measured every column withlipgloss.Widthand padded withpadColbefore joining with a gap. Four copies of the same twenty lines, and the copy insideprintTasksalso carried the terminal-width column dropping. There is now onetableininternal/output/table.go.Rows of cells go in, measured and padded lines come out. The column dropping is a
dropOrderfield naming the columns that may be given up, in the order they go, rather than a pair of booleans and a hand-written width sum.The rule the four copies shared is written down once: every column is padded to its widest cell except the last one declared, which goes out as it is. Dropping a column does not move that rule, which is why a narrow listing still pads its title column. That is the behaviour the copies had; it is now stated rather than implied.
printTaskskeeps its group-header logic, which no table can own, because it has to write a header between two rows.table.lines()hands back one line per row for exactly that, so the header fold from #205 is untouched.One place each for the kind word and the status text
t.Type == model.TypeProjectwas branched on six times acrossoutput.goandagent.go. It is nowisProject(t), and the word that reaches the output iskindWord(t), which takes "task" and "project" from model's type codec, the same source the JSONtypefield renders from.kindWordis deliberately notTaskType.String(). A heading, or a code Things has yet to write, reads as a task on these surfaces, which is what the CLI has always shown. The doc comment says so.statusTextno longer keeps its own switch over the three statuses. It capitalisesmodel.Status.String(), so the detail block, the JSON and the agent brief cannot drift apart, and the unrecognised code still renders "Unknown" because the codec already falls back to "unknown".checklistLinetakes its "(cancelled)" from the same source.statusIconstays as it is.[ ],[~]and[x]are glyphs rather than the status words, and Markdown's two-state checkbox in a brief is a third thing again. Folding those together would have meant inventing a rendering, which this change is not for.Proof the output did not change
The golden test went in first, in its own commit, before anything was refactored.
TestRenderGoldenrenders a fixed fixture through every public entry point:Printfor tasks, projects, areas and tags and for a*model.Task,PrintTaskListwith and without a view label,PrintTaskWithChecklistfor a to-do, a project and a repeating item,PrintHint, andPrintAgentBrieffor a task, an open project, an empty project, a closed project, a repeating item and a note carrying its own code fence. Empty listings are cases too. Each case renders at both ends of the colour profile, so the ANSI is pinned as well as the text, and the task listings render at three terminal widths so both drop steps are covered.The refactor commit does not touch
testdata/render_golden.txt. That is the claim, and the file's absence from that commit's diff is the evidence.The golden values were captured from unmodified rendering code. The one production change in that first commit is the seam the test needs:
termWidthbecomes a var, likenowFnbeside it, so a test can pin a width instead of depending on how the test binary's stdout happens to be attached. The body is unchanged.origin/mainmoved to 61542c3 while this was in review, but #260 does not touchinternal/output, so the captured bytes are still the bytes of the base this merges onto.Against the live database, 226 invocations of the built binary are byte-identical to
origin/main, 1.8 MB of output in all: nine views plain, with--include-completed, as JSON, with--color alwaysand with--no-hints;projects,areasandtagsin the same three forms; and 43 real items throughshow,show --agent,show --jsonandshow --color always. Exit codes are compared too. The database was read only through the binary.Every line in the golden file ends in
|, because a padded column emits trailing spaces an editor or a hook would otherwise eat, and ANSI escapes are written\eso the file stays readable and greppable.Review
/code-review --fixat high effort found one defect, and it was mine, in the golden test rather than in the refactor.TestRenderGoldenreturned early when the document matched and otherwise reported from inside its per-case loop. A difference the case splitter does not see, such as trailing whitespace or text before the first header, both of which it drops, left the loop with nothing to report and the test passed withgot != want. Appending two newlines to the golden file reproduced it. For a test whose whole job is pinning bytes that is the one outcome it must not have, so it now fails loudly and names the regeneration command.The review separately re-derived the extraction rather than trusting the tests: that
table.fitsreproduces the old arithmetic including the two-step drop and the absent re-check after the last drop, that the last declared column is the one left unpadded whether or not it survived, thatkindWordandstatusTextare equivalent to the switches they replace down to the unknown-code arm, and that the golden file is the same blob in both commits. It also confirmed the golden output is timezone-stable, checked underTZ=Pacific/KiritimatiandTZ=UTC, and that makingtermWidtha var introduces no race.Noted, not fixed
TestListQueryGoldenSQLininternal/dbhas the same gap the review found here: it reports only from inside its per-case loop, so a difference outside a### view=block would pass. It is a separate package and a separate change, so it is left alone.Not touched
No command surface moves, so
internal/skill/SKILL.mdand the pages underdocs/content/have nothing to say about this. The(project)marker, the group header fold, the project icons and the agent brief's wording are all documented contracts and all unchanged.