refactor(documents): make the document builders public (#212)#216
Merged
Conversation
documents.py is the library-level read-model projection layer, and its docstring says outright that a non-CLI frontend (the planned web backend) imports these builders directly. Every builder nonetheless kept the leading underscore it had as a private function inside cli.py, which says the opposite. The six *_SCHEMA_VERSION constants beside them were already public; the split had no rationale, it was an artifact of the origin. Deferred out of #211 because that PR's deliverable was a provable verbatim relocation (AST-identical bodies, untouched tests as the proof) — renaming in the same commit would have voided both. run_token_totals goes public too. The issue framed it as an open question, on the grounds that it is a shared helper rather than document API and could stay a private sibling. It cannot: cmd_status imports it across the module boundary and calls it on the text path, and a private name imported by another module is the exact contradiction being removed. The run_ prefix stays — it marks run-level vs per-task aggregation, which is what the function exists to get right. No back-compat cli._x_document aliases: nothing in-tree needs them and they would re-create the private surface this removes (same call the Unreleased notes already record for runs.tmux_sessions -> mux_sessions). No __all__: no leaf module in the package has one, including probe.py and diagnostics.py, which this module is modelled on. No new CHANGELOG entry — nothing user-visible changes — but the Unreleased entry naming _run_token_totals is updated, since it is unshipped notes rather than history. Also adds the library path's first coverage. Every other --json test reaches the builders through cli.main, so nothing pinned the docstring's central promise that the two paths agree. Two parity tests drive one fixture down both and assert the same dict comes back. Equality is against the raw builder return, not a json round-trip: a consumer holds this dict before serializing, so a tuple where a list belongs is a real defect a round-trip would hide — verified by injecting exactly that and watching the test fail. conftest.make_validate_document (new in #215) was already calling the builder directly, so it needed the rename too. Note make_validate_document contains _validate_document as a substring — the rename is word-boundary anchored so it and its 13 call sites are untouched.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThe JSON document builders in ChangesPublic document API
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
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
1 participant
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 #212.
documents.pyis the library-level read-model projection layer, and its docstring says outright that a non-CLI frontend (the planned web backend) imports these builders directly. Every builder nonetheless kept the leading underscore it had as a private function insidecli.py— which says the opposite. The six*_SCHEMA_VERSIONconstants beside them were already public; the split had no rationale, it was an artifact of the origin.Deferred out of #211 because that PR's deliverable was a provable verbatim relocation (AST-identical bodies, untouched tests as the proof) — renaming in the same commit would have voided both.
The one judgement call
run_token_totalsgoes public too. The issue framed this as open, on the grounds that it is a shared helper rather than document API and could stay "a private sibling within its own module." It cannot:cmd_statusimports it across the module boundary (cli.py:53) and calls it on the text path (cli.py:1581), after the--jsonearly return. A private name imported by another module is the exact contradiction being removed.The
run_prefix stays — it marks run-level vs per-task aggregation, which is the distinction the function exists to get right.Decisions
cli._x_documentaliases. Nothing in-tree needs them and they would re-create the private surface this removes. Same call the Unreleased notes already record forruns.tmux_sessions→mux_sessions("internal, no deprecation aliases").__all__. No leaf module in the package has one, includingprobe.pyanddiagnostics.py— the modules this one is modelled on. Adding one here alone would make the absence there newly ambiguous._run_token_totalsis updated, since it is unshipped notes rather than history.New: the library path's first coverage
Every other
--jsontest reaches the builders throughcli.main, so nothing pinned the docstring's central promise — that the library path and the CLI path agree. Two parity tests drive one fixture down both and assert the same dict comes back.Equality is asserted against the raw builder return, not a json round-trip: a consumer holds this dict before serializing it, so a tuple where a list belongs is a real defect a round-trip would hide. Verified by injecting exactly that (
"tasks": tuple(tasks)) and confirming the test fails, then reverting.Scoped to
status/list, which have clean deterministic fixtures.clean/cleanupneed mutation fixtures andvalidateneeds a drivenValidationReport— those would balloon this past a rename.Notes for review
tests/conftest.pywas the surprise:make_validate_document, new in feat(tui): render validate --json in a findings modal (#210) #215, already calls the builder directly — so an in-tree consumer was already reaching into a private name across a module boundary, which strengthens the issue's case. It needed the rename too.make_validate_documentcontains_validate_documentas a substring. The rename is word-boundary anchored, so it and its 13 call sites intest_tui_app.pyare untouched — verified by dry-run diff before applying.tests/test_cli.pyhas one docstring-only cross-ref fix; zero assertion changes to any pre-existing test, so refactor(cli): extract the --json document builders into documents.py #211's "the untouched tests are the proof" story survives.cli.py:36-47is deliberately byte-identical: all sixnoqa: F401are load-bearing (ruff's autofix silently deletes a re-export), and the comment above them stays accurate. Checked withtrunk check --no-fix.Verification
2628 passed, 1 skippedtrunk check --no-fix— no issuescli.*_SCHEMA_VERSIONre-exports still resolve;noqa: F401count still 6Summary by CodeRabbit
--jsonoutput.