Add Diff.apply and Diff.revert; add doc annotations#7
Conversation
There was a problem hiding this comment.
Build & Tests
Build: pass (local, ARM Linux).
Tests: pass — all 34 tests pass, including the 12 new apply/revert tests.
CI: pass — both macOS and Ubuntu green.
Findings
Clean PR. Reviewed the implementation against the diff algorithm and traced through edge cases:
-
Roundtrip correctness.
applycollectsEq+Insertionelements,revertcollectsEq+Deletion— this is the correct reconstruction. Tests verify the roundtrip property (apply (diff old new) == newandrevert (diff old new) == old) for equal arrays, insertion-only, deletion-only, empty, and prefix/suffix cases. -
Concatenation cost.
Array.concat &[res (Array.copy xs)]inside the loop is O(n·k) total where n is the number of hunks and k is total element count. This matches the idiomatic Carp pattern (no in-place extend), same as other array-building loops in the codebase. Not a concern for typical diff sizes. -
Doc annotations. All accurate.
diff,string-diff,eq?,inserted?,deleted?,eq,insertions,deletionsnow havedocentries — these were the only public functions missing them. -
No CHANGELOG in this repo, so nothing to update.
No bugs or edge case failures found.
Verdict: merge
Correct, well-tested, fits the existing codebase style. Good addition.
hellerve
left a comment
There was a problem hiding this comment.
The tests make it seem as if it’s equivalent to take the second array. What are the subtleties here? And if it’s more complex, shouldn’t there be test cases for it?
Construct diffs directly (not via diff) to show apply/revert work on the diff structure alone. Int and String variants with interleaved Eq/Insertion/Deletion hunks demonstrate that the two functions produce meaningfully different results.
|
@hellerve Good point — the existing tests all follow The subtlety is that
These make it clear that All 38 tests pass. |
There was a problem hiding this comment.
Build & Tests
Build: pass (ARM Linux, local).
Tests: all 38 pass, including the 4 new hand-built mixed diff tests.
CI: Ubuntu passes. macOS fails in the Setup Carp step (Haskell stack download returns a corrupt archive: tar: Error opening archive: Unrecognized archive format). This is a transient infrastructure issue — the previous CI run on this same PR passed on both platforms. Not caused by this PR's changes.
Prior feedback
- hellerve noted the original tests looked like they just "take the second array" and asked for tests demonstrating real complexity. The agent responded with 4 hand-built mixed diff tests that construct diffs directly (no
diffcall) with interleaved Eq/Insertion/Deletion hunks, showingapplyandrevertproduce genuinely different results from the same structure. This addresses the concern well. - carpentry-reviewer gave a "merge" verdict on the prior round when CI was green. The macOS failure appeared on the subsequent push and is unrelated (infra).
Findings
Traced through both implementations manually. apply collects Eq+Insertion, revert collects Eq+Deletion — correct by definition. The Array.concat &[res (Array.copy xs)] pattern matches the style used in diff itself (line 47-51). No bugs found.
Minor observation: the tests don't cover single-element hunks, adjacent same-type hunks, or empty inner hunks like (Eq []), but I verified these all work correctly — adding them is optional.
Verdict: merge
Code is correct, well-tested, idiomatic. The macOS CI failure is a flaky external dependency (Haskell stack installer), not a code regression — re-running CI should clear it.
Summary
Diff.apply— reconstructs the "new" array from a diff (collects Eq and Insertion elements)Diff.revert— reconstructs the "old" array from a diff (collects Eq and Deletion elements)docannotations for all public functions that were missing them:diff,string-diff,eq?,inserted?,deleted?,eq,insertions,deletionsThe diff library could compute diffs but had no way to apply them back, which is a real usability gap. These two functions close that gap. The doc annotations make the library easier to discover via
(doc ...)and generated documentation.Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.