feat(cli): add --json output to register, content, and pop commands#76
Conversation
EnderOfWorlds007
requested review from
andrew-ifrita,
corey-hathaway,
filvecchiato,
sphamjoli and
waytwotall
as code owners
CI Summary
Release - PassedTest this PR Download artifact (GitHub CLI required): gh run download 24500185212 -n cli-release-0.0.0-pr.76 -R paritytech/dotns-sdkInstall globally: npm install -g ./parity-dotns-cli-0.0.0-pr.76.tgzVerify: dotns --helpDeploy Example — Passed
Labelspkg: cli, type: test Test - Passed142 passed, 0 failed across 142 tests. |
|
@andrew-ifrita @sphamjoli — This replaces #74 with the reviewer feedback addressed: Re: contentHash.ts console leak (@andrew-ifrita) — Investigated this thoroughly. Re: automated tests (@andrew-ifrita @sphamjoli) — Added both unit and integration tests:
Re: CI permissions (@andrew-ifrita) — Pushed directly to the repo instead of from fork. CI should work now. Additional fix: Switched JSON output from |
EnderOfWorlds007
force-pushed
the
feat/json-output-all-commands
branch
from
ae158d2 to
ce753f8
Compare
There was a problem hiding this comment.
Howsit, nice work getting --json across all the commands. A few things to tighten up before merge:
A quick run with claude produces
- The error handling block is copy-pasted ~10 times
Every command handler has the same catch block (format error, check jsonOutput, write to stderr/stdout, exit). This should be a single shared function. Right now if we need to change the error shape, we'd have to touch 10+ places.
Locations: content.ts, pop.ts, registerCommand.ts, lookup.ts , every .action() handler.
- The success output pattern is also duplicated
The if (jsonOutput) console.log(JSON.stringify(result)) else console.log(chalk.green(...)); process.exit(0) block repeats in every handler too. Same fix, Ideally extract once, call everywhere.
getMergedOptions()is identical in two files
content.ts:20-37 and pop.ts:23-41 have the same function. Should live in one place.
getJsonFlag()has a suspicious triple fallback (lookup.ts:35-46)
It tries optsWithGlobals(), then opts(), then falls back to process.argv.includes("--json"). If Commander options are wired correctly, the process.argv fallback shouldn't be needed. Worth investigating whether this is papering over an option propagation issue.
maybeQuiet()lives in bulletin.ts
It's a general-purpose utility imported by content, pop, register, and lookup, but it's defined in the bulletin command module. If we're keeping it, it should live in a shared location.
- Error format is inconsistent
content.ts and pop.ts use chalk.red("\n✗ Error: ...") while registerCommand.ts uses chalk.red.bold("✗ Error:") with different string structure. Extracting the error handler (point 1) would fix this automatically.
- JSON envelope shapes are inconsistent
Some responses include ok: true (register, pop set, content set), others don't (content view, pop info). It wpuld be be good to pick a convention, e.g., mutations return ok, reads don't, and document it.
- Test files repeat the same patterns
The --json help tests in contentJson.test.ts, popJson.test.ts, and registerJson.test.ts are identical except for the command name. A shared helper would reduce the boilerplate. Same for the keystore setup/teardown block that's duplicated across all integration test files.
|
Thanks for the thorough review @sphamjoli — good observations across the board. Let me separate what's in scope for this PR vs what predates it: Will fix in this PR (items 1, 2, 7):
Pre-existing — suggest a follow-up PR (items 3, 4, 5, 6, 8):
Does that split work for you, or would you prefer we tackle everything in one go? |
We can give it a go in one i dont see the benefit of multiple PRs in this case cc: @andrew-ifrita WDYT? |
One PR is fine in order to include I suspect a good amount of AI gen here (which is fine), but I encourage having your model review the PR first before pulling in humans. |
|
@EnderOfWorlds007 whats the status on this PR? |
Move getMergedOptions (6 copies), getJsonFlag, withCapturedConsole, and maybeQuiet into jsonHelpers.ts as the single shared location. Remove the process.argv fallback from getJsonFlag — Commander option propagation handles this correctly. Extract expectJsonHelpOption test helper to reduce boilerplate across --json unit tests. Addresses review items 3-5, 8 from PR #76.
|
@sphamjoli @andrew-ifrita — All review items addressed in a single PR as requested. Here's the breakdown: Items 1 & 2 (error/success duplication): Extracted Item 3 (getMergedOptions duplication): Removed from all 6 files (content.ts, pop.ts, bulletin.ts, text.ts, auth.ts, info.ts) and consolidated into Item 4 (getJsonFlag process.argv fallback): Removed the Item 5 (maybeQuiet in bulletin.ts): Moved Item 7 (JSON envelope convention): Documented in Item 8 (test boilerplate): Extracted Net result of the refactoring commit: -103 lines (150 added, 253 removed) across 14 files. All checks pass locally (typecheck, 142 unit tests, lint, prettier). |
Move getMergedOptions (6 copies), getJsonFlag, withCapturedConsole, and maybeQuiet into jsonHelpers.ts as the single shared location. Remove the process.argv fallback from getJsonFlag — Commander option propagation handles this correctly. Extract expectJsonHelpOption test helper to reduce boilerplate across --json unit tests. Addresses review items 3-5, 8 from PR #76.
EnderOfWorlds007
force-pushed
the
feat/json-output-all-commands
branch
from
335e5b9 to
0f79ee8
Compare
Add structured JSON output support to all CLI commands that were missing it: register domain, register subname, content set, content view, pop set, and pop info. This follows the same pattern already used by the bulletin and lookup commands -- the --json flag suppresses human-readable output via maybeQuiet() and writes a single JSON line to stdout. The underlying contentHash functions now return result objects (domain, cid, contenthash, txHash) and the registration functions return result objects (label, domain, owner) so the CLI layer can serialize them. Errors are written as JSON to stderr when --json is active.
Switch --json output from process.stdout.write/process.stderr.write to console.log/console.error, aligning with the existing pattern in lookup.ts. This ensures the test harness captures JSON output correctly. Add unit tests verifying --json flag in help output and JSON error formatting for pre-chain validation errors. Add integration tests for content view/set, pop info/set, and register domain in --json mode, validating JSON shape and absence of human output markers. Add code comment explaining why ora spinners are safe inside maybeQuiet (withCapturedConsole replaces .write on the stream object that ora caches by reference).
Move getMergedOptions (6 copies), getJsonFlag, withCapturedConsole, and maybeQuiet into jsonHelpers.ts as the single shared location. Remove the process.argv fallback from getJsonFlag — Commander option propagation handles this correctly. Extract expectJsonHelpOption test helper to reduce boilerplate across --json unit tests. Addresses review items 3-5, 8 from PR #76.
…edicate The empty-contenthash check (0x, 0x0, length < 6) was duplicated between decodeContenthashToCid and the viewDomainContentHash return. Derive hasContent from the decoded string instead.
EnderOfWorlds007
force-pushed
the
feat/json-output-all-commands
branch
from
0f79ee8 to
be48ae3
Compare
|
(with help of AI) I believe edit: This would be with the console.log banners edit edit: I think these are suppressed by |
andrew-ifrita
left a comment
andrew-ifrita
left a comment
There was a problem hiding this comment.
Looks sufficient. Thanks for putting this together
## Summary Mirrors #76 for the `text` namespace — the last on-chain CLI commands that didn't have machine-readable output. Unblocks `bulletin-deploy` #173 (set name + description text records at deploy time). Follows the exact pattern that #76 established for `register`/`content`/`pop`: - CLI wrapper uses `getJsonFlag` / `maybeQuiet` / `emitJsonResult` / `handleCommandError` from `jsonHelpers.ts`. - Underlying `viewDomainText` / `setDomainText` now return structured results (`TextViewResult` / `TextSetResult`) — same shift `viewDomainContentHash` / `setDomainContentHash` did in #76. - `--json` flag suppresses chalk/ora output via `withCapturedConsole`. - Banner is already gated by `process.argv.includes("--json")` in `program.ts`, so it's silent under `--json`. ## JSON output shapes ``` dotns text view <name> <key> --json → { "domain": "....dot", "key": "...", "exists": true, "owner": "0x...", "value": "..." | null } # unregistered domain: → { "domain": "....dot", "key": "...", "exists": false, "owner": null, "value": null } dotns text set <name> <key> <value> --json → { "ok": true, "domain": "....dot", "key": "...", "value": "...", "txHash": "0x..." } # any error path: → stderr: { "error": "<message>" }, exit 1 ``` Reads return data directly (no `ok` field); writes return `{ ok: true, ... }`. Matches the envelope convention documented in `jsonHelpers.ts`. ## Behaviour change worth flagging `text view` previously had a manual piped-stdout fallback: when stdout was a pipe, all chalk output got rerouted to stderr and the bare value was written to stdout, so `dotns text view foo bar | xargs ...` would just receive the value. That hack is removed — it predated `--json` and conflicted with the new flag (the redirect would hijack the JSON envelope). Scripted users should now use `--json` and parse the `value` field. The non-piped, non-json TTY experience is unchanged. ## Tests - New unit file `tests/unit/text/textJson.test.ts` (3 tests): `--json` flag in help output for both subcommands + JSON error envelope for the pre-chain `--mnemonic` + `--key-uri` mutex. Mirrors `contentJson.test.ts`. - New `--json` cases appended to `tests/integration/text/text.test.ts` (4 tests): registered-domain happy path for view/set, unregistered-domain returning `exists: false` (view) and JSON error (set). Mirrors the `--json` block at the bottom of `content.test.ts`. - `bun test tests/unit/` → **163/163 pass** (was 159; +4 from this PR — 3 new + the 1 textHelp test count is unchanged but textJson adds 3, totaling +4? actually +4 unit assertions across the 3 tests; verified locally). - `bun run build` → green. - `bun run lint` → clean. - `bun run format` → clean. - `bun run typecheck` → 47 pre-existing errors (same count on `main`); zero introduced by this PR. ## Scope Deliberately excluded from this PR — should be follow-ups: - `account address`, `account info`, `account map` — self-contained namespace, separate concern from text records. - `auth set/list/use/remove/clear` — keystore management, involves interactive password prompts; needs its own design pass. Closes the upstream gap noted in bulletin-deploy PR #180 (`### #173 / text records — NOT in this PR`).
3 participants
Summary
Adds
--jsonflag to 6 commands that lacked it (content view,content set,pop set,pop info,register domain,register subname). Thebulletinandlookupcommands already had--json.Replaces #74 (fork PR → upstream branch to fix CI permissions).
Changes from #74
process.stdout.writetoconsole.log(andprocess.stderr.writetoconsole.error) to align with the existing--jsonpattern inlookup.ts. The test harness only interceptsconsole.log/console.error, so the previous approach wouldn't have been captured in tests.maybeQuiet:withCapturedConsolereplaces.writeon theprocess.stderrobject, and ora caches the stream object reference (not the method), so spinner output is captured when the spinner lifecycle runs insidemaybeQuiet.--jsonflag in help output, JSON error formatting for pre-chain validation errors.content set.JSON output shapes
Reviewer feedback addressed
withCapturedConsoleinterceptsprocess.stderr.write(not justconsole.log), so ora spinner output is captured whenmaybeQuietwraps the call scope. Added code comment explaining the mechanism.--jsoncommands.Test plan
bun test tests/unit/(142 tests, 0 failures)