You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
PR #88 (3645f4e) implements issue #84, the specification that supersedes #74. The #84 scope includes $super / $super* composition, deferred grounding, and marker-family validation in inheritance filename slots.
Executive summary
Request changes. The core array-composition algorithm is good: it keeps $super deltas pending, composes splice with splice, rejects both directions in which $super* meets a marked delta, grounds after inheritance, and follows the pairing kind table. It passes all #78–#81 and #85 positive tests, plus the targeted regression that exposed PR #81's deferred-flattening bug.
However, PR #88 violates #84's filename grammar. raw:$super is not unescaped before filename resolution, and $super? is accepted as an optional filename rather than rejected as an unknown marker. With a file named $super present, $super? silently includes it. This is a functional specification defect, not merely an error-message mismatch.
Evidence
Reviewed head: 3645f4e, based on 1956c65.
go test ./..., go test -race ./..., go vet ./..., and git diff --check pass.
make build and the full autotest suite pass: 166 passed, 0 failed.
The PR changes about 240 production lines, adds 11 autotests (4 normal, 7 negative), and adds no feature-focused Go unit tests.
Direct probes showed {"$extends":["raw:$super"]} fails even when a sibling $super file exists, and {"$extends":["$super?"]} succeeds by resolving that file.
1. Implementation
1.1 Correctness
mergeArrayComposition correctly preserves default replacement, carries a marked child that has no inherited value, rejects a non-array inherited value, composes $super with $super, and rejects all combinations involving a $super* delta. pairSuper and mergeArrayPair correctly implement objects as deep merges, atoms and unmarked arrays as child-wins, marked child arrays as recursive composition, and cross-kind pairs as errors.
GroundArrayMarkers correctly handles object keys, scalar values, nested arrays, unknown spellings, and raw: values or keys. Its placement in the command pipeline is correct: after file- and node-level inheritance, before key-side evaluation.
The blocking exception is filename handling. parseInheritsField does not validate marker strings, LoadAndResolveInheritancesRecursively handles ? before grounding, and inheritance directives are removed before the final grounding pass. The result is that raw: cannot escape a literal filename, exact marker filenames can be loaded if they exist, and $super? bypasses the unknown-marker rule. #84 explicitly requires validation and raw: in this position.
1.2 Design clarity
The separation between mergeObjectsForInheritance, mergeArrayComposition, splice, pairing, and grounding is understandable, and the reverse splice-over-pairing check is correctly explicit. The design is less cohesive than #85/#86 because the 226-line feature is added to internal/json.go and lives beside the older errorless mergeObjects / MergeObjects API. A future caller could easily use ordinary merging where inheritance semantics are required.
Filename parsing should be part of the marker-validation design: validate each inheritance slot before optional-file processing, then strip raw: before resolution.
1.3 Readability
Names and comments are generally clear. The main weaknesses are missing structural paths in errors and nondeterministic first errors when multiple invalid map entries exist. The latter also obscures broad negative fixtures.
1.4 Efficiency
Splice and pairing allocate fresh outer slices without deep-copying elements, which is the right cache-safety trade-off. Pairing capacity is well bounded. Splice preallocates only for a single marker despite supporting many markers, and repeated marker scans add small linear overhead; neither is likely material without a benchmark.
2. Tests
2.1 Coverage
The 4 positive autotests cover splice placement/repetition, an $includes mixin, one associativity layout, and object pairing with prefix, queue, padding, and tail. The 7 negatives cover unresolved and mixed markers, one marked-delta direction, a non-array inherited value, three cross-kind pairs, an object-key/nested-array context fixture, and an unknown spelling.
This is a useful start but much less complete than #85's 28 or #86's 26 cases. Missing coverage includes raw and unknown filename slots, raw keys, scalar context, duplicate $super*, the two other pairing-delta directions, node-level composition, recursively marked child arrays, all six cross-kind pairs, both associativity layouts, and the later-base forbidden-composition regression.
The cross-kind and out-of-context tests put several invalid values in one map. Because execution stops at the first map entry, they do not independently prove every claimed case. No Go unit tests cover classification, fresh-slice behavior, merge rules, or grounding.
2.2 Design clarity
The directory names and short fixtures are readable. The aggregate fixtures reduce diagnostic value and make coverage appear broader than it is; individual rule fixtures would form a much clearer acceptance contract.
2.3 Readability
Normal examples communicate the syntax well, and negative expectations use stable categories rather than complete text. Focused unit-test names are the main missing maintenance aid.
3. Documentation
3.1 Readability and organization
evaluation-model.adoc is strong: it documents opt-in composition, splice, pairing, deltas, and adds grounding at the correct lifecycle point.
terminology.adoc has a structural AsciiDoc error. The new === Array Composition heading appears between a [source,json] opener and its closing delimiter, so the heading and prose render as source code rather than a terminology section. The raw example should be closed before the new heading.
make doc could not render locally because the gendoc executable is absent. The source-layout error is directly visible without rendering.
3.2 Correctness
The evaluation-model text accurately describes the core algorithm. It describes raw: only for values and keys, omitting the required filename escape; this matches the incomplete implementation rather than #84.
Verdict
Request changes before merge. PR #88 has a correct core engine and avoids the central defects in #79 and #81, but filename marker handling is part of the agreed language grammar and currently violates it. Add filename validation/normalization, individual tests for every marker context, a small feature-unit-test layer, and repair the AsciiDoc block. After that fix it could be a competitive simpler implementation; as submitted, #85 and #86 remain the safer current-spec candidates.
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
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.
Implements #84 in the specified phases.
$supersplice composition, deferred deltas, grounding, and marker-family validation.$super*index-wise pairing, same-kind enforcement, and marked-delta composition errors.Array replacement remains the default; composition is opt-in per array.
Validation:
go test ./...,make build && tools/bin/autotest(166/166),make doc, andgit diff --check.