🧹 Janitor: Refactor JSON output to use emit_json API - #289
Conversation
…rinting
Extracts a generic `emit_json` function in `commands.rs` to replace the highly duplicated `println!("{}", serde_json::to_string(payload)?)` boilerplate scattered throughout the CLI commands for the `--json` flag output.
Co-authored-by: matta <37314+matta@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Merging to
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the application's JSON output mechanism by introducing a dedicated Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively refactors the JSON output logic by introducing a centralized emit_json function, which improves code reuse and maintainability. The changes are applied consistently across the codebase.
I have two main points of feedback:
- A critical issue regarding an extraneous patch file (
patch_commands.diff) that seems to have been accidentally included in the commit. This file should be removed. - A suggestion to improve the performance of the new
emit_jsonfunction by writing directly tostdoutinstead of creating an intermediate string.
Overall, this is a good cleanup. Addressing these points will make the PR ready for merging.
| <<<<<<< SEARCH | ||
| /// Resolved runtime configuration and paths for command execution. | ||
| pub struct RunContext { | ||
| ======= | ||
| /// Emits a JSON-serializable payload to standard output. | ||
| /// | ||
| /// Serializes the provided generic payload as a JSON string and writes it to `stdout` | ||
| /// followed by a newline. This centralizes the standard pattern of converting | ||
| /// structured data into machine-readable output for CLI commands. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `payload` - A reference to any type that implements `serde::Serialize`. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns an error if serialization fails (e.g., if the type contains a map with | ||
| /// non-string keys, though this is rare for standard API objects). | ||
| pub fn emit_json<T: Serialize>(payload: &T) -> Result<()> { | ||
| println!("{}", serde_json::to_string(payload)?); | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Resolved runtime configuration and paths for command execution. | ||
| pub struct RunContext { | ||
| >>>>>>> REPLACE |
There was a problem hiding this comment.
This file appears to be a patch file, possibly an artifact from an automated tool, and contains what look like merge conflict markers. It should not be committed to the repository. Please remove this file from the pull request.
| /// Returns an error if serialization fails (e.g., if the type contains a map with | ||
| /// non-string keys, though this is rare for standard API objects). | ||
| pub fn emit_json<T: Serialize>(payload: &T) -> Result<()> { | ||
| println!("{}", serde_json::to_string(payload)?); |
There was a problem hiding this comment.
For better performance, consider serializing the JSON payload directly to stdout instead of creating an intermediate string. Using serde_json::to_writer with a locked stdout handle avoids the allocation of a potentially large string, which can be more efficient.
let stdout = io::stdout();
let mut handle = stdout.lock();
serde_json::to_writer(&mut handle, payload)?;
writeln!(handle)?;1 participant
🎯 Goal: Factoring common logic into modular APIs
💡 Before:
println!("{}", serde_json::to_string(payload)?)was duplicated everywhere✨ After: Extracted a reusable generic function
commands::emit_json(payload)✅ Verification: Ran full
just testandjust checkto ensure correctnessPR created automatically by Jules for task 14434972392799991463 started by @matta