refactor(compilers/openapi): define the yaml-node builders once - #373
Merged
Conversation
internal/nodeview and internal/scan each carried their own copy of the same six yaml-node builders, byte for byte in four cases, and mergeChainSpec was a seventh copy shared between internal/scan and the compiler's own tests. The copies survived because ymerge needs the merge tag and the tag lived in nodeview: a home above nodeview would be one nodeview's own internal tests could not import, since an internal test file cannot import a package that imports its own package. Rather than split the family across two homes, the tag moves down instead. compilers/openapi/internal/ynode holds MergeTag and the constructors that spell the node shapes it names, nodeview imports it for IsMergeKey, and both packages' tests build nodes from the one definition. MergeTag is a fact about yaml.v3 and speakeasy rather than about the view, so it reads no worse one level down, and the predicate that tests for it keeps the comment explaining what the tag means. ynode carries its own tests: without -coverpkg a package is instrumented only by its own test binary, so one with statements and no test files contributes zero-count blocks to the profile and fails the coverage gate.
sourceindex landed while this branch was open, carrying its own copy of the four node builders — byte for byte the ones ynode now defines. The branch consolidated the copies it could see, so merging main left the count at one rather than none, and "define the builders once" no longer described the tree. sourceindex's internal tests build the same nodes for the same reason the view's and the scan's do, and ynode sits below it as it sits below them, so the same argument applies unchanged: the definitions go and the call sites name ynode. Its allowlist is left alone, unlike nodeview's. archtest reads production sources only, and only nodeview imports ynode from one; sourceindex reaches it from an internal test, which the layering check never sees. Listing the edge there would claim a dependency the checked graph does not have.
OmarAlJarrah
force-pushed
the
refactor/openapi-shared-yaml-builders
branch
from
August 13, 2026 10:23
038ee09 to
bf3bf2a
Compare
OmarAlJarrah
added a commit
that referenced
this pull request
Aug 13, 2026
The branch forked before #328, #356 and #373, so three of the four files needed more than a textual resolution: - The test helpers it calls were consolidated while it was open. ymap, yscalar, yalias and ymerge became ynode.Map/Scalar/Alias/Merge in #373, and yamlNode became openapitest.YAMLNode in #328. The bodies are identical, so these are renames. - TestRawChildNode_IsNotTheMergeAwareView asserted that a repeated key resolves to opposite ends in the two readers. #356 made RawChildNode take the last pair, as the parser does, so they now agree. The case is rewritten to assert the agreement rather than deleted: a reader drifting back to first-wins is worth failing on. - nodeview.go kept main's DocumentPath, walkPointer and tokenless, and main's ynode.MergeTag over the local const this branch predates. The index is additive to all of it.
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
internal/nodeviewandinternal/scaneach carried their own copy of the same sixyaml-node builders —
yscalar,ymap,yseq,yalias,ymerge,mergeChain— byte forbyte in four cases.
mergeChainSpecwas a seventh copy, shared betweeninternal/scanandthe compiler package's own tests.
internal/sourceindexlanded while this branch was open with a copy of its own — four ofthe same builders, byte for byte — so it is folded in here too. Its allowlist is left alone,
unlike
nodeview's:archtestreads production sources only, andsourceindexreachesynodefrom an internal test, so listing the edge would claim a dependency the checked graphdoes not have.
Direction taken: one definition, not a comment explaining why there are two.
The issue offered both and asked for a decision. What kept the family apart was that
ymergeneeds the merge tag and the tag lived innodeview, so any shared home would havehad to import
nodeview— andnodeview's own tests are one of the two callers. Thatconstraint is real, and I checked it by construction rather than by reading: a probe package
importing
nodeview, imported fromnodeview's internal test file, fails to build withBut the constraint only blocks a home above
nodeview. Moving the tag below it removesthe problem entirely, and the same probe inverted — a package
nodeviewimports, importedfrom
nodeview's internal test — builds and passes. So the family gets a home:compilers/openapi/internal/ynodeholdsMergeTagand the constructors that spell the nodeshapes it names,
nodeview.IsMergeKeyreads the tag back, and both packages' tests buildnodes from the one definition.
MergeTagis a fact about yaml.v3 and speakeasy'syml.IsMergeKey, not about the view, soit reads no worse one level down.
IsMergeKeyitself stays innodeview— it is the view'spredicate, used by the view's expansion — and keeps the comment explaining what the tag
means and why the check is spelled the way it is.
mergeChainSpecmoves too, asynode.MergeChainSpec: it is the same merge-chain fixture insource form, spelling the same
<<chain whose tag the package defines.ynodegets its own entry ininternal/archtest'srulesmap (gopkg.in/yaml.v3andnothing else) and
nodeview's entry gains it, with the import constraint written down asthe reason the package sits where it does. It also gets its own tests: without
-coverpkgapackage is instrumented only by its own test binary, so one with statements and no test
files contributes zero-count blocks to the profile and fails the coverage gate — confirmed
against the probe package, which reported
coverage: 0.0%and put a zero-count block in theprofile while its only callers were elsewhere.
Test plan
Every builder is defined exactly once. The issue's own grep now matches nothing:
This is a test-infrastructure change, so the risk is a silently weakened test rather than a
broken one. Three things were checked:
The moved builders produce identical nodes. A throwaway test compiled the pre-move
definitions verbatim from
git HEADalongside the moved ones and compared their output —cmp.Diffover every scalar value the callers use, a nested tree built by both sets, andMergeChain/MergeChainSpecat levels 0, 1, 2, 3, 7, 64, 66, 200 and 1600. Green.MergeChainSpec's body also diffs clean against the original line for line.No test changed meaning. Comparing
go test ./... -list '.*'before and after, the onlydifference is additions — 7 new tests, all in the new package, zero removals or renames
(1270 names before, 1277 after).
The new tests were checked against planted defects rather than read:
MergeTagbecomes"!!MERGE"ynode'sTestMerge_MatchesTheParsedMergeKey,nodeview'sTestIsMergeKey_AgreesWithParsedTagsandTestMappingPairs_MergeKeySources,scan'sTestDetectCycles_ReproducersMergeChain's leaf pair becomes{leaf: w}ynode'sTestMergeChain_NestsOneMergePerLevelOverALeafandnodeview'sTestNodeView_TruncationIsPerNodeMergeChainSpecstops writingpaths: {}That first row is the load-bearing one: the mutation reddens production behaviour in
nodeviewandscan, not just the new package's own tests, which is what shows the movedconstant is still the one
IsMergeKeyreads.The third row is why an assertion was added. Dropping
paths: {}from the fixture reddenednothing in the repository — 3.1 makes
pathsoptional, and the fixture still parsed,compiled and satisfied both callers.
TestMergeChainSpec_AnchorsEveryLevelAndNamesItFromASchemanow asserts the preamble key by key, and re-planting the defect reddens it.
Full gate green, coverage at exactly 100% including the new package. The statement count is elided from the transcript below: the gate's total depends on test-cache warmth (#369), so it would describe the cache rather than the branch.
Out of scope
pairMapis still a per-package copy innodeviewandscan. It takes[]nodeview.Pair,so a shared home for it would have to import
nodeview— the cycle above, and not onemoving a constant can fix.
cycleReproducersandreadReproducerare also duplicatedbetween
internal/scanand the compiler package's tests; both are unrelated to the mergetag.
Closes #325