Replace nwidger/jsoncolor with an in-tree colorizer#5170
Replace nwidger/jsoncolor with an in-tree colorizer#5170
Conversation
fancyJSON was the last caller of nwidger/jsoncolor and was the only thing forcing fatih/color.Color values across a package boundary. Removing it unblocks a future fatih/color migration. Replace it with a small token walker over json.MarshalIndent output that emits the same ANSI palette (green strings, cyan numbers, bold-green true, red false, magenta null, bold-blue keys). defaultRenderer.renderJson gates colorization on cmdio TTY/color capabilities; pretty_json template helper stays on !color.NoColor for parity with the other helpers in renderFuncMap. Acceptance test outputs regenerated: encoding/json.MarshalIndent emits ": " (colon + space) where jsoncolor emitted a bare ":". Bundled with this change is one test.toml redaction regex update for the new spacing. Co-authored-by: Isaac
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
Deep review (Isaac + Cursor, 2-round swarm)
Verdict: Approved — 0 Critical, 0 Major, 1 Gap (Suggestion), 5 Nits, 4 Suggestions.
Both reviewers independently concluded the implementation approach is sound: tokenize json.MarshalIndent output byte-by-byte, wrap tokens in ANSI, leave punctuation/whitespace untouched, and gate on cmdio capabilities. The round-trip property test is the right invariant. All findings below are nits or follow-up suggestions; nothing blocks merging.
Inline comments follow. Items note which reviewer surfaced them and whether the other confirmed in cross-review.
[Suggestion] Follow-up: colorize iterator JSON output too (Isaac, acknowledged by Cursor)
Out of scope for this PR, but worth a follow-up task: iteratorRenderer.renderJson (unchanged by this PR, around libs/cmdio/render.go:107) still emits uncolored json.MarshalIndent. After this PR, single-object JSON on a TTY is colorized but list JSON on the same TTY is not — a pre-existing inconsistency. Since colorizeJSON accepts any json.MarshalIndent-shaped bytes, a follow-up is small: run it over per-item bytes plus the surrounding [, ,\n , ] punctuation when stdout capabilities allow color.
- Add Capabilities.SupportsStdoutColor() helper, use at the call site. - Add renderer-level test (TestRenderJSONColorGate) covering all 4 combinations of stdoutIsTTY and color. - Cite SGR Wikipedia reference above the ANSI const block. - Bump bytes.Buffer Grow hint from len(b)/4 to len(b)/2 (per-token overhead is ~9-11 bytes, dense JSON exceeded the old hint). - Note in scanNumber doc that it assumes valid JSON. - Rename TestMarshalJSONWithoutColorMatchesEncodingJSON to ...MarshalIndent. - Tighten changelog entry: clarify it's the single-object render path (iterator output already used encoding/json spacing). Co-authored-by: Isaac
2 participants
Changes
Drop
github.com/nwidger/jsoncolorand replacefancyJSONwith a small in-tree colorizer overjson.MarshalIndentoutput. Same ANSI palette as before (green strings, cyan numbers, bold-greentrue, redfalse, magentanull, bold-blue keys).defaultRenderer.renderJsonnow gates colorization on cmdio TTY/color capabilities;pretty_jsontemplate helper stays on!color.NoColorfor parity with the other helpers inrenderFuncMap.Why
fancyJSONwas the last caller ofnwidger/jsoncolor, and it was the only thing forcingfatih/color.Colorvalues across a package boundary. Removing it unblocks a futurefatih/colormigration and replaces the incidental "color is off because fatih's package init saw stdout isn't a TTY" gating with explicit cmdio capability checks.Tests
libs/cmdio/jsoncolor_test.gocover string/number/literal tokens, escape sequences, key vs value, empty containers, and a round-trip property test (stripping ANSI yields the original bytes).databricks current-user me -o jsonon a TTY shows the same colors as before; piped orNO_COLOR=1produces plain JSON.PR description drafted with Claude Code.