Skip to content

Add Diff.apply and Diff.revert; add doc annotations#7

Merged
hellerve merged 2 commits into
masterfrom
claude/apply-revert-docs
Jun 8, 2026
Merged

Add Diff.apply and Diff.revert; add doc annotations#7
hellerve merged 2 commits into
masterfrom
claude/apply-revert-docs

Conversation

@carpentry-agent

Copy link
Copy Markdown

Summary

  • Add Diff.apply — reconstructs the "new" array from a diff (collects Eq and Insertion elements)
  • Add Diff.revert — reconstructs the "old" array from a diff (collects Eq and Deletion elements)
  • Add doc annotations for all public functions that were missing them: diff, string-diff, eq?, inserted?, deleted?, eq, insertions, deletions
  • Add tests for both new functions covering equal arrays, insertions-only, deletions-only, empty diffs, and prefix/suffix cases

The 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.

@carpentry-reviewer carpentry-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Roundtrip correctness. apply collects Eq + Insertion elements, revert collects Eq + Deletion — this is the correct reconstruction. Tests verify the roundtrip property (apply (diff old new) == new and revert (diff old new) == old) for equal arrays, insertion-only, deletion-only, empty, and prefix/suffix cases.

  2. 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.

  3. Doc annotations. All accurate. diff, string-diff, eq?, inserted?, deleted?, eq, insertions, deletions now have doc entries — these were the only public functions missing them.

  4. 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 hellerve left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@carpentry-agent

Copy link
Copy Markdown
Author

@hellerve Good point — the existing tests all follow apply (diff old new) == new, which does just look like "take the second array."

The subtlety is that apply/revert work on the diff structure alone — you can serialize a diff, transmit or store it, and reconstruct either side without the original arrays. To demonstrate this, I added 4 tests that construct diffs by hand (no diff call) with interleaved Eq/Insertion/Deletion hunks:

  • Int mixed diff: [(Eq [1 2]) (Insertion [10 20]) (Deletion [3]) (Eq [4]) (Deletion [5 6]) (Insertion [30])]apply produces [1 2 10 20 4 30], revert produces [1 2 3 4 5 6]
  • String mixed diff: [(Eq ["hello"]) (Deletion ["cruel" "old"]) (Insertion ["brave" "new"]) (Eq ["world"])]apply gives ["hello" "brave" "new" "world"], revert gives ["hello" "cruel" "old" "world"]

These make it clear that apply and revert are doing real work — they produce different results from the same diff by selecting different hunk types.

All 38 tests pass.

@carpentry-reviewer carpentry-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 diff call) with interleaved Eq/Insertion/Deletion hunks, showing apply and revert produce 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.

@hellerve hellerve merged commit 9871e63 into master Jun 8, 2026
3 of 4 checks passed
@hellerve hellerve deleted the claude/apply-revert-docs branch June 8, 2026 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant