Closed
Conversation
Adds serde/serde_json dependencies and a --json flag to the list subcommand. When passed, entries serialize as a JSON array instead of the human-readable table. Includes integration tests for valid JSON output and filtered JSON output. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User (CLI)"
participant Main as "tccutil-rs (main)"
participant TccDb as "TccDb"
participant Serializer as "serde_json"
User->>Main: invoke `--user list [--client X] [--service Y] [--json]`
Main->>TccDb: list(client_filter, service_filter)
TccDb-->>Main: Vec<TccEntry>
alt --json flag set
Main->>Serializer: to_string_pretty(Vec<TccEntry>)
Serializer-->>Main: JSON string
Main-->>User: print JSON
else
Main->>Main: print_entries(Vec<TccEntry>) (pretty text)
Main-->>User: print human-readable output
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Cargo.toml (1)
19-23: Redundant dev-dependency.
serde_jsonis listed in both[dependencies](Line 20) and[dev-dependencies](Line 23). When a crate is in[dependencies], it's automatically available for tests, making the dev-dependency entry redundant.♻️ Proposed fix
[dev-dependencies] -serde_json = "1" tempfile = "3"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` around lines 19 - 23, Remove the redundant dev-dependency entry for serde_json: since serde_json is already declared under [dependencies] (serde_json = "1"), delete the duplicate serde_json = "1" line from the [dev-dependencies] section so tests use the main dependency; ensure only one declaration of serde_json remains in Cargo.toml.src/main.rs (1)
422-431: Consider the interaction between--jsonand--compactflags.When both
--jsonand--compactare specified,--jsontakes precedence and--compactis silently ignored. The JSON output always includes the fullclientpath since it serializesTccEntrydirectly.This behavior is reasonable (JSON consumers can transform the data as needed), but you may want to either:
- Document this in the
--jsonhelp text, or- Make the flags mutually exclusive using clap's
conflicts_withOption: Make flags mutually exclusive
/// Compact mode: show only binary name instead of full path - #[arg(short, long)] + #[arg(short, long, conflicts_with = "json")] compact: bool,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 422 - 431, The --json and --compact flags currently interact so that --json wins and --compact is ignored; update the flag definitions (the clap Arg for "json" or "compact" in the CLI setup) to make them mutually exclusive by adding conflicts_with("compact") on the "json" arg (or conflicts_with("json") on "compact") so the parser rejects using both; alternatively, if you prefer to keep both allowed, update the help text for the "json" flag to explicitly note that JSON output serializes TccEntry fully and that --compact is ignored (the relevant symbols are the json and compact boolean flags, the TccEntry serialization, and print_entries).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Cargo.toml`:
- Around line 19-23: Remove the redundant dev-dependency entry for serde_json:
since serde_json is already declared under [dependencies] (serde_json = "1"),
delete the duplicate serde_json = "1" line from the [dev-dependencies] section
so tests use the main dependency; ensure only one declaration of serde_json
remains in Cargo.toml.
In `@src/main.rs`:
- Around line 422-431: The --json and --compact flags currently interact so that
--json wins and --compact is ignored; update the flag definitions (the clap Arg
for "json" or "compact" in the CLI setup) to make them mutually exclusive by
adding conflicts_with("compact") on the "json" arg (or conflicts_with("json") on
"compact") so the parser rejects using both; alternatively, if you prefer to
keep both allowed, update the help text for the "json" flag to explicitly note
that JSON output serializes TccEntry fully and that --compact is ignored (the
relevant symbols are the json and compact boolean flags, the TccEntry
serialization, and print_entries).
…with json Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main.rs (1)
230-237: Consider adding a test for the--compactand--jsonconflict.The test verifies
--jsonparses correctly, but there's no test ensuringconflicts_with = "json"works. Consider adding a test that asserts combining both flags produces an error:🧪 Suggested test for argument conflict
#[test] fn parse_list_compact_and_json_conflict() { let err = parse(&["tcc", "list", "--compact", "--json"]).unwrap_err(); assert_eq!(err.kind(), ErrorKind::ArgumentConflict); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 230 - 237, Add a new unit test named parse_list_compact_and_json_conflict that calls parse(&["tcc", "list", "--compact", "--json"]) and asserts it returns an error whose kind() is ErrorKind::ArgumentConflict; this verifies the conflicts_with = "json" on the List variant (see parse function and Commands::List) actually prevents using --compact and --json together.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main.rs`:
- Around line 230-237: Add a new unit test named
parse_list_compact_and_json_conflict that calls parse(&["tcc", "list",
"--compact", "--json"]) and asserts it returns an error whose kind() is
ErrorKind::ArgumentConflict; this verifies the conflicts_with = "json" on the
List variant (see parse function and Commands::List) actually prevents using
--compact and --json together.
Co-Authored-By: Claude Opus 4.6 <[email protected]>
3 tasks
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main.rs (1)
428-438: Consider graceful error handling instead of.expect().While serialization of
TccEntry(containing onlyString,i32,bool) should never fail in practice, using.expect()will produce a panic with a stack trace if it ever does. For consistency with the error handling pattern used elsewhere in this function (lines 439-442), consider usingmatchorunwrap_or_elseto print a clean error and exit.♻️ Optional: Graceful error handling
Ok(entries) => { if json { - println!( - "{}", - serde_json::to_string_pretty(&entries) - .expect("failed to serialize entries") - ); + match serde_json::to_string_pretty(&entries) { + Ok(json_str) => println!("{}", json_str), + Err(e) => { + eprintln!("{}: failed to serialize entries: {}", "Error".red().bold(), e); + process::exit(1); + } + } } else { print_entries(&entries, compact); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 428 - 438, The current JSON branch uses serde_json::to_string_pretty(&entries).expect(...) which panics on serialization failure; change it to handle errors gracefully by matching the Result (or using unwrap_or_else) from serde_json::to_string_pretty and on Err print a clean error message (including the error) to stderr and exit with a non-zero status instead of panicking; keep the JSON printing behavior on Ok and otherwise mirror the existing error handling used elsewhere in this function (see usage of print_entries, json, compact) so the program logs the serialization failure and exits cleanly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main.rs`:
- Around line 428-438: The current JSON branch uses
serde_json::to_string_pretty(&entries).expect(...) which panics on serialization
failure; change it to handle errors gracefully by matching the Result (or using
unwrap_or_else) from serde_json::to_string_pretty and on Err print a clean error
message (including the error) to stderr and exit with a non-zero status instead
of panicking; keep the JSON printing behavior on Ok and otherwise mirror the
existing error handling used elsewhere in this function (see usage of
print_entries, json, compact) so the program logs the serialization failure and
exits cleanly.
…ation Match the error handling pattern used elsewhere in main(): print to stderr with colored "Error" prefix and exit(1) instead of panicking with a stack trace. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Contributor
Author
|
Closing this PR to re-implement the same feature with Codex. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a
--jsonflag to thelistsubcommand for machine-readable output.Changes
serde+serde_jsondependenciesSerializeonTccEntryand sub-types--jsonbool flag toListcommand — outputs a JSON array when set, existing table output unchangedUsage
Summary by CodeRabbit
New Features
Tests
Documentation