refactor(engine): extract RowSlots helper for weights/children guard#85
Merged
Merged
Conversation
Polish pass on PR-7.3's defence-in-depth IllegalArgumentException added in v1.6.5 (commit f868499). The guard lived inline as duplicated code at two engine call sites — LayoutCompiler#distributeRowSlotWidths and NodeDefinitionSupport#measureRow — with no direct test. The path is unreachable through public API (RowNode canonical constructor blocks the mismatched state), but unreachable code that lacks coverage rots: the next refactor can silently delete either guard with no test going red. Changes: - New com.demcha.compose.document.layout.RowSlots (package-private) with a single helper, validateWeightsMatchChildren(weights, n). Error message unchanged from PR-7.3. - Both call sites replaced with the helper call. - New RowSlotsTest with 4 tests: happy path, more-weights-than-children, fewer-weights-than-children, and a message-quality check that the error tells the caller how to fix it. - GraphCompose.DocumentBuilder#pageBackgrounds(...) Javadoc now spells out the empty-list-clears semantics in prose, not just in @param. Inner comment in DocumentBuilder.create() shortened — the rationale moved up to the public Javadoc where it belongs. No behaviour change. Suite: 1023 tests, 0 failures, 0 errors (~47s). Surfaced by retrospective senior-review of PR-7.3 via the new graphcompose-senior-review skill.
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.
Summary
Polish pass surfaced by retrospective senior review of PR-7.3 (
f868499b) through the newgraphcompose-senior-reviewskill. PR-7.3 added a defence-in-depthIllegalArgumentExceptionguard for the row weights / children size mismatch at two engine call sites — and shipped no test that drives those guards directly, because the path is unreachable through the public API (RowNode's canonical constructor blocks the mismatched state at construction time).Unreachable code without coverage rots: the next engine refactor can silently delete either guard with no test going red. This PR locks the contract behind a covered surface.
Changes
RowSlots(src/main/java/com/demcha/compose/document/layout/RowSlots.java, package-private) with a single helper,validateWeightsMatchChildren(weights, n). Package-private intentionally — engine surface, not public API. Error message unchanged from PR-7.3.LayoutCompiler#distributeRowSlotWidths(was lines 746-750) andNodeDefinitionSupport#measureRow(was lines 324-328) now delegate toRowSlots.validateWeightsMatchChildren(...).RowSlotsTest(src/test/java/com/demcha/compose/document/layout/RowSlotsTest.java) with 4 tests:containsAnyOf("Pass","Provide","Use")) without locking the exact wording.GraphCompose.DocumentBuilder#pageBackgrounds(...)— empty-list-clears semantics now spelled out in a<p>block instead of only the@paramline. Inner comment inDocumentBuilder.create()shortened — rationale moved up to the public Javadoc where it belongs.### Engine internals (no behaviour change)subsection under## v1.6.6 — Planned.Why this PR, not a comment on PR-7.3
PR-7.3 shipped in v1.6.5. The guard was correct and the public-API surface is right — what was missing is the test-the-helper scaffolding that survives a future refactor. Fixing it forward as a small isolated PR (zero behaviour change,
japicmpclean) is cheaper than retroactively reopening a closed release.The retrospective senior review also flagged the duplicated error message as a future-drift risk (PR-7.1 lesson generalised: any literal in two files is a drift waiting to happen). The helper kills the duplication in one move.
Verification
The new
binary-compatCI job (PR-84) will checkjapicmpagainstv1.6.5— expectingNo changessince the helper is package-private and the public-API contract is identical.Test plan
RowSlotsTestcovers happy + both negative directions + message qualityNo changes(package-private extraction = no public-surface delta)Senior review findings addressed
RowSlots+ 4-testRowSlotsTestcovers it directlyRowSlots; both sites delegateGraphCompose.javainner comment + builder Javadoc don't surface the empty-list-clear contract in prose<p>block added to builder Javadoc; inner comment shortened