refactor: make overall CLI act more expected and consistent, and add quiet flag#78
refactor: make overall CLI act more expected and consistent, and add quiet flag#78
Conversation
…json & quiet flag
bartlomieju
left a comment
There was a problem hiding this comment.
Good direction overall — the --quiet flag, non-interactive detection, consistent error() usage, and UX polish (green checkmarks, examples, aliases) are all welcome improvements.
A few things I noticed:
Bug fix 👍
The connection string validation logic fix (|| → &&) in database.ts:415-416 is a real bug fix — the old code threw on valid postgres:// URLs. Worth calling out in the PR description.
as any casts in sandbox/mod.ts
The 10 new as any casts on .command() calls (lines 1108-1144) are a type regression. This is likely caused by moving -q, --quiet from individual subcommands to globalOption, creating a type mismatch between SandboxContext (which may not include quiet) and the parent command's type. Worth investigating whether updating the SandboxContext type to include quiet would fix this cleanly rather than suppressing the errors.
quiet type
GlobalContext.quiet is typed as quiet?: true (literal true) in main.ts:20. Should be quiet?: boolean — the current type means you can't do context.quiet = false, which is fine in practice but unusual. A boolean type is more conventional and matches how cliffy options work.
Scattered if (!quiet) checks in publish.ts
publish.ts has ~15 individual if (!quiet) guards. Consider extracting a small helper like:
const log = quiet ? () => {} : console.log.bind(console);Or wrapping the Spinner in a helper that no-ops when quiet. This would reduce the noise and make it harder to miss a guard.
isInteractive vs requireInteractive
Both are exported from util.ts but requireInteractive calls error() which calls Deno.exit(1). The isInteractive check in auth.ts:183 duplicates this pattern manually. Could just use requireInteractive there too for consistency.
Lock file churn
The lock file diff is large (~100 lines) from new transitive deps. This seems like it might be from a deno.json change not shown in the diff, or a Deno version bump. Worth confirming these are intentional.
Minor
env loadgained--replaceand--skip-existingflags — nice addition for CI, but the--replaceflag has no validation that it conflicts with--skip-existing.- The "environmental" → "environment" typo fixes throughout
env.tsare correct 👍
Fixes denoland/deno#33003