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
› Please work on this issue: https://github.com/dakusui/jqplusplus/issues/84
Try not to ask me questions as much as possible but figure out questions from what you can see on the issue and what you have in the repo.
Commit changes for each phase and for change after the defined phases.
Prefix the pull request's name with {your model name such as sol, terra, or luna}-{effort level}: but please do not merge it as I will do that by myself.
Also, post a comment at the end of the work to record the tokens consumed for this work.
Implements the specified opt-in array composition feature in delivery order:
Phase 0: specification autotests for splice, pairing, deltas, grounding, reserved namespace, and cross-kind errors.
PR #85 (ed18ef4) implements the now-current specification in issue #84, which supersedes #74. The #84 specification preserves the core $super / $super* behavior
and additionally makes marker-family strings invalid outside a direct array
element.
Executive summary
This is an excellent implementation and the strongest candidate reviewed so
far against the current specification. It fixes the original terra-xhigh
PR #79's central splice-delta error, implements both directions of the
pairing/splice incompatibility, and has the most complete Phase-0 autotest
suite. I found no semantic defect in the reviewed scope.
The main remaining concerns are modest: there is no benchmark; validation error
selection is nondeterministic when several map keys are invalid; and one
important negative autotest does not include the later base array needed to
prove rejection occurs at the delta-composition boundary rather than only at
final grounding.
Evidence
go test ./..., go test -race ./..., and go vet ./... pass.
The full autotest suite passes: 183 passed, 0 failed.
The core merge logic accurately models the specification's marker state rules:
$super child over $super parent is permitted and carries a new splice
delta.
$super* child over either kind of marked parent fails.
$super child over $super* parent also fails immediately.
Pairing follows the same-kind table: objects deep-merge, atoms are
child-wins, unmarked arrays replace, marked child arrays recurse, and every
cross-kind pair errors.
Unresolved deltas are carried only while inheritance may still bind them;
grounding runs after structural inheritance and before evaluation.
The stricter #84 validation is implemented well. It rejects exact markers in
scalar values, object keys, nested arrays, and inheritance filename slots; it
also reserves non-identifier $super extensions while allowing identifier
continuations and raw: escapes. Earlier PRs did not implement this full
context validation because it was clarified in #84.
No correctness flaw emerged from the prior positive suites or the targeted
pairing/splice regression. This is confidence, not proof of completeness.
1.2 Design clarity
internal/arraycomposition.go is a focused home for the feature. Its central composeSuperArray function makes the important transition rule explicit:
pairing cannot compose with any marked parent, and splice cannot compose over a
pairing parent. This directly avoids the over-broad #79 guard and the missing
reverse-direction guard in #81.
The context-aware final validator is also a good fit for #84. The markerContext enum keeps the expanded syntax rule understandable instead of
spreading special cases across parsing and evaluation.
Two minor design costs remain. MergeObjects now returns an error everywhere,
and its pre-existing MergePolicy parameter still does not select distinct
array behavior. Also, pairwise errors can acquire a duplicated path prefix,
for example at .items: at .items[0].
1.3 Readability
The code is clear and well commented. Names distinguish splice, pairing,
grounding, and marker context consistently, which is especially valuable after #84's terminology discipline. Error messages describe the failed semantic rule
instead of merely reporting an implementation condition.
The only readability weakness is the validation walk's two passes over direct
array strings: one pass validates/counts and the next validates recursively.
It is harmless but initially makes the control flow look more complex than it
is.
1.4 Efficiency
Like #80, #85 always creates a fresh outer slice when composing arrays but does
not deep-copy immutable elements. That is the right cache-safety/performance
trade-off for this codebase and should be more efficient than #78's recursive
copies.
There are small opportunities: splice capacity does not use its already-known
marker count, pairwise capacity can over-allocate, and validation rescans direct
array strings. None is likely material for normal configuration sizes; no
benchmark supports a quantitative ranking.
2. Tests
2.1 Coverage
The autotest suite is the best of the candidates. Positive cases cover append,
prepend, wrapping with multiple markers, node-level inheritance, a mixin,
flat includes, chained composition, prefix/queue behavior, padding/tails, and
both value- and key-side raw: escapes. Negative cases cover dangling markers,
mixed/duplicate markers, both pairing/splice directions, every cross-kind pair,
unknown spellings, and all #84 out-of-context positions.
The unit suite covers the same core laws, including fresh-slice behavior,
recursive marked arrays, and both incompatible marker directions. The previous
Fable bug is explicitly covered as "splice over pairing."
One small gap remains: the super-splice-over-pairing autotest has no later
base array. It proves that the immediate pair/splice merge errors, but a test
with a later base is stronger because it catches an implementation that flattens
the delta, delays validation, and only appears correct when no base arrives.
2.2 Design clarity
This is a genuinely specification-shaped test suite. Each end-to-end directory
names one behavior, the suite follows the Phase-0 checklist in #84, and the
negative cases make the expanded context grammar executable. It is much easier
to use as a canonical acceptance suite than the earlier aggregate autotests.
2.3 Readability
Fixtures are small and self-contained. Error expectations use focused
substrings, which avoid over-coupling to presentation while still documenting
the failure category. The unit tests use clear subtest names; their map-based
iteration makes execution order nondeterministic, but the cases are independent.
3. Documentation
3.1 Readability and organization
Documentation is very good. It adds the required terms to terminology.adoc,
links the evaluation model and syntax reference to those definitions, and gives
the reader a clear progression: default replacement, composition, deferred
deltas, grounding, and errors. The reference supplies concise examples for
both operations and the summary table improves discoverability.
3.2 Correctness
The documentation matches #84's important refinements: any composition that
involves a pairing delta errors; exact markers outside direct array elements
are out of context; unknown non-identifier family spellings are rejected; and raw: provides an escape. It documents the array-pair recursion rule that PR #81 omitted.
I reviewed the AsciiDoc source. The local documentation generator could not be
completed because its Docker image acquisition did not finish in this
environment, so this is not a rendered-document verification claim.
Verdict
Recommended. If the choice is made against the current #84 specification,
PR #85 is ahead of PR #80: it keeps #80's correct marker-state behavior while
adding the agreed context validation, a more systematic autotest suite, and a
terminology-aligned documentation set.
Before merging, I would add one final autotest: the $super* parent → $super
child case with a later concrete base array. That locks in the boundary that
previously let PR #81 appear correct under ordinary test suites.
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.
Closes #84.
Prompt
Implements the specified opt-in array composition feature in delivery order:
$supersplice composition, carry semantics, grounding, and reserved-namespace validation.$super*pairing, same-kind pair rules, recursive marked-array composition, and incompatible-delta errors.Validation:
go test ./...tools/bin/autotest(183 passed)The pull request is intentionally not merged.