Add global --output flag with table, json, and csv support

[514sid]

Summary

• Replaces 14 per-subcommand --json flags with a single global --output <table|json|csv> flag — consistent interface across all commands
• CSV output is opt-in per formatter; commands with nested data (e.g. edge-app setting list) reject it with a clear error rather than producing malformed output
• --json is kept as a hidden deprecated alias with a warning to avoid breaking existing scripts during migration

[sergey-borovkov]

Code Review

Overview

Replaces 14 per-subcommand --json flags with a single global --output {table|json|csv} flag. CSV is opt-in per formatter (Formatter::supports_csv) and commands that don't support it (e.g. edge-app setting list with nested arrays) reject it with a clear error. --json is kept as a hidden deprecated alias guarded with conflicts_with = "output". Solid, focused refactor — the surface-area cleanup is a clear win.

Strengths

• Clean UX consolidation. One global flag is the right shape; propagate_version + global = true make it work uniformly.
• Safe deprecation path. Hidden --json + conflicts_with means existing scripts keep working but --json --output ... fails fast instead of silently picking one.
• CSV opt-in via trait default. supports_csv() -> false by default forces explicit acknowledgment per formatter, and the error message on unsupported types is good.
• Helpful README section including the RUST_LOG=off note for clean pipes.

Issues & Suggestions

Correctness / consistency

• playlist get was not migrated (src/cli.rs:698-709). It still hand-rolls serde_json::to_string_pretty and ignores --output entirely. README claims "All list and get commands support three output formats" — this command violates that. It will silently emit JSON regardless of --output table or --output csv. Either migrate it through handle_command_execution_result (add a PlaylistFile formatter) or explicitly reject non-json output here.

• Deprecation warning uses warn! (src/cli.rs:540). This routes through env_logger. The README tells users to set RUST_LOG=off to get clean CSV/JSON output to a file — which will swallow the deprecation warning too. Users migrating from --json won't see it in exactly the scenario where they're most likely to hit it. Use eprintln! so the warning is always visible on stderr and doesn't pollute the stdout payload.

• CSV numeric formatting (src/commands/mod.rs:90-95). The chain is as_str → as_bool → as_u64 → as_f64. Negative integers fail as_u64 and fall through to as_f64, becoming \"-1.0\" instead of \"-1\". Try as_i64 between as_u64 and as_f64.

• Single-object Get commands and CSV. format_value only iterates if value.value().as_array() succeeds. If any Get returns a bare object (not wrapped in an array), CSV will be a lone header row with no data. Worth confirming screen get / asset get actually return arrays in the current code path; otherwise add an "object → single-row" branch.

Polish

• OutputFormat derives. Consider adding Copy (it's a tiny enum) so you can pass it by value instead of &OutputFormat through every handle_cli_*_command — would tidy up the call sites in cli.rs.

• pub field widening on Cli. output and json are now pub (previously json: Option<bool> was private). Minor API surface increase; if Cli is only consumed in main.rs/handle_cli, prefer pub(crate).

• Empty struct variants. List {} reads slightly awkwardly; List (unit variant) is the idiomatic form since there are no fields. Pure aesthetic, no behavior change.

• println! of CSV string. The formatter returns the full CSV from csv::Writer (which already ends each record with \r\n) and println! adds another \n. Minor — most CSV consumers don't care, but print! would produce a cleaner stream.

Test coverage

Net +191 / −154 with no new unit tests. Worth adding at least:

• OutputFormat::Csv round-trip for one supporting type (e.g. Screens or Assets) — header row + row escaping for values containing commas/quotes.
• The "CSV not supported" rejection path for EdgeAppSettings.
• That --json still routes to JSON and emits the deprecation warning (capture stderr).
• --json --output json fails with clap's conflict error.

Security / performance

• No security concerns. CSV is in-memory via Vec<u8> and small list payloads; no path traversal, no shell-out.
• Adding the csv crate (~minor footprint) is justified given hand-rolled CSV escaping is error-prone.

Risk Summary

Low-to-moderate. The deprecated-alias migration is well-handled. The two things to fix before merging:

1. playlist get ignores --output — either migrate or document the exception.
2. Deprecation warn! will be silenced by RUST_LOG=off — switch to eprintln!.

Everything else is polish. Recommend addressing those two, optionally adding a couple of focused tests, then ship.

[514sid]

@sergey-borovkov

playlist get was not migrated (src/cli.rs:698-709). It still hand-rolls serde_json::to_string_pretty and ignores --output entirely. README claims "All list and get commands support three output formats" — this command violates that. It will silently emit JSON regardless of --output table or --output csv. Either migrate it through handle_command_execution_result (add a PlaylistFile formatter) or explicitly reject non-json output here.

f04f768

Deprecation warning uses warn! (src/cli.rs:540). This routes through env_logger. The README tells users to set RUST_LOG=off to get clean CSV/JSON output to a file — which will swallow the deprecation warning too. Users migrating from --json won't see it in exactly the scenario where they're most likely to hit it. Use eprintln! so the warning is always visible on stderr and doesn't pollute the stdout payload.

fb91a34

CSV numeric formatting (src/commands/mod.rs:90-95). The chain is as_str → as_bool → as_u64 → as_f64. Negative integers fail as_u64 and fall through to as_f64, becoming "-1.0" instead of "-1". Try as_i64 between as_u64 and as_f64.

8e38c89

OutputFormat derives. Consider adding Copy (it's a tiny enum) so you can pass it by value instead of &OutputFormat through every handle_cli_*_command — would tidy up the call sites in cli.rs.

bd6ba39

pub field widening on Cli. output and json are now pub (previously json: Option was private). Minor API surface increase; if Cli is only consumed in main.rs/handle_cli, prefer pub(crate).

d361c82

Empty struct variants. List {} reads slightly awkwardly; List (unit variant) is the idiomatic form since there are no fields. Pure aesthetic, no behavior change.

f52a821

println! of CSV string. The formatter returns the full CSV from csv::Writer (which already ends each record with \r\n) and println! adds another \n. Minor — most CSV consumers don't care, but print! would produce a cleaner stream.

31ab7e8

Test coverage

c6aa5fa

[sergey-borovkov]

Thanks for the quick turnaround — all major points addressed:

• ✅ playlist get migrated, full Formatter for PlaylistFile (table / json / csv)
• ✅ warn! → eprintln! so the deprecation warning survives RUST_LOG=off
• ✅ as_i64 added to the CSV numeric chain
• ✅ OutputFormat: Copy + pub(crate), passed by value
• ✅ Empty struct variants → unit variants
• ✅ print! for CSV to avoid the trailing newline
• ✅ Tests: CSV round-trip with comma/quote escaping, EdgeAppSettings rejection, --json flag, --json --output json conflict

Two small remaining nits, both non-blocking:

1. PlaylistFile CSV uses snake_case headers (asset_id, duration) while every other formatter reuses column_names and emits display-case headers (Id, Title, ...). For consistency I'd use ["Asset Id", "Duration"] here too — scripts can still parse it, and it matches the rest of the codebase.

2. PlaylistFile table/CSV drop predicate and playlist_id. JSON preserves them, but table/CSV silently omit them. Could be a follow-up — show them as a small header above the items table, or as additional columns in CSV.

LGTM otherwise.

[sergey-borovkov]

LGTM — all major review points addressed, remaining nits are non-blocking.