Add from-json interface and to-json for Map/Maybe#3
Conversation
There was a problem hiding this comment.
Build & Tests
CI passes on both macos-latest and ubuntu-latest. All 201 tests pass (163 existing + 38 new). No local Carp compiler available for independent build verification.
Findings
Reviewed the full implementation (json.carp:780-891) and all new tests (test/json.carp:752-935).
1. to-json additions (json.carp:781-793) — Correct.
Map.to-json: useskv-reduceto build(Map String (Box JSON)), copies values with@vand boxes them. Keys passed by ref toMap.put. MatchesArray.to-jsonpattern. ✓Maybe.to-json:Nothing→JSON.Null,Just x→(to-json x). Clean. ✓
2. from-json interface (json.carp:803) — (Fn [(Ref JSON)] (Result a String)) is the right signature. Takes (Ref JSON) to avoid consuming input. Type variable a resolved per implementation. ✓
3. Primitive from-json implementations (json.carp:806-840) — All use match-ref correctly. Bool, Double, String are straightforward. ✓
4. Int/Long from-json (json.carp:813-828) — Double.to-int and Double.to-long silently truncate. A JSON number like 1e20 will overflow Int without error; NaN/Infinity produce undefined results. The truncation test (line ~790 in tests) verifies 3.7 → 3, which is good. This is a reasonable design tradeoff for a first version, but worth documenting — callers should know conversion is lossy and unbounded.
5. let [_ 0] in Map.from-json (json.carp:876) — This appears to be a workaround for Carp's borrow checker: without the let binding, val may not have the right lifetime scope for &val in Map.put. The workaround works but would benefit from a short comment explaining why it's needed (e.g., ; extend val lifetime for &val borrow), since it looks like dead code to anyone unfamiliar with the quirk.
6. Array.from-json short-circuit (json.carp:847-862) — Correct. On error, Result.Error is propagated through remaining iterations. Box.peek only borrows, no leak. ✓
7. Maybe.from-json (json.carp:883-891) — Correct for single-level Maybe. Note: Maybe (Maybe Int) has an inherent JSON limitation — null maps to Nothing, so Just Nothing is unrepresentable. This is the standard tradeoff (Aeson, serde, etc.) and is fine, but worth a doc note eventually.
8. Missing test coverage (non-blocking):
- No empty array
from-jsontest (emptyJSON.Arr []) - No empty map
from-jsontest (emptyJSON.Obj) - No roundtrip tests for
MaporMaybe(roundtrips only cover Int, String, Bool, Array at lines 911-931) - No
Map.from-jsontest with a bad element value (only tests top-level type mismatch)
9. No CHANGELOG — The repo has no CHANGELOG file, so this is not a gap introduced by this PR.
Verdict: merge
Solid implementation completing the JSON round-trip story. CI green on both platforms, 38 new tests with good coverage. The let [_ 0] workaround and missing edge-case tests are minor — suitable for a follow-up, not a blocker. The Int/Long overflow behavior is a reasonable first-cut design tradeoff.
What
Adds the
from-jsoninterface for converting JSON values back to Carp types, and completes theto-jsonstory by adding implementations forMapandMaybe.to-jsonadditionsMap.to-json— converts(Map String a)toJSON.Obj, whereaimplementsto-jsonMaybe.to-json— converts(Maybe.Nothing)toJSON.Nulland(Maybe.Just x)to(to-json x)from-jsoninterface and implementationsNew interface:
(definterface from-json (Fn [(Ref JSON)] (Result a String)))Implementations for all core types:
JSON.BoolJSON.NumviaDouble.to-int(truncates toward zero)JSON.NumviaDouble.to-longJSON.NumviaDouble.to-floatJSON.NumJSON.StrJSON.Arr, converting each element; propagates first errorJSON.Obj, converting each value; propagates first errorJSON.Null→Nothing, otherwise tries innerfrom-jsonand wraps inJustAll implementations return
(Result.Error "expected <type>")on type mismatch.Why
The library had
to-jsonfor 7 types but nofrom-jsonat all. Round-trip JSON serialization is fundamental for practical use — this completes the type conversion story.Tests
38 new tests covering:
to-jsonfor Map and Maybefrom-jsonfor every type (success + type mismatch)to-json→from-jsonrecovers original value)All 201 tests pass.
carp-fmtandanglerclean.Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.