format output - #4
Conversation
There was a problem hiding this comment.
Pull request overview
This PR cleans up the output behavior of the buildgraph CLI tool: it migrates diagnostic/progress messages from the log package (which prefixes timestamps) to fmt.Fprintf(os.Stderr, ...), removes internal-detail fields from the JSON output, and changes the default output format from JSON to text.
Changes:
logreplaced withfmt.Fprintf(os.Stderr, ...)/fmt.Fprintln(os.Stderr, ...)across all CLI files for cleaner, timestamp-free stderr output.Impact.AffectedFunctionsandImpact.AffectReasonsexcluded from JSON serialization (json:"-");Impact.Changesfield removed from both the type and its initialization.- Default output format for
analyzechanged from"json"to"text", and the format flag description updated accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
cli/analyze.go |
Removed leftover log progress/summary messages; changed default --format flag from "json" to "text" |
cli/output.go |
Replaced log.Fatalf with fmt.Fprintf+os.Exit(1); removed fixed-width padding from change type in text output |
cli/pipeline.go |
Replaced all log.Printf calls with fmt.Fprintf(os.Stderr, ...) |
cli/root.go |
Replaced log.Fatalf with fmt.Fprintf+os.Exit(1) in config loading |
pkg/types/types.go |
Excluded AffectedFunctions and AffectReasons from JSON; removed Changes field from Impact |
pkg/impact/impact.go |
Removed initialization of the now-deleted Changes field in ComputeImpact |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| func init() { | ||
| analyzeCmd.Flags().StringP("format", "f", "json", "Output format: json, text") | ||
| analyzeCmd.Flags().StringP("format", "f", "text", "Output format: text, json") |
There was a problem hiding this comment.
The README.md states that JSON is the default output format (# JSON output (default)) for buildgraph analyze, but this PR changes the default from "json" to "text". The README documentation is now incorrect and should be updated to reflect that text is the new default (i.e., swap the examples so text is the default and json requires --format json).
| AffectedFunctions map[string][]string `json:"-"` | ||
| AffectReasons map[string][]string `json:"-"` |
There was a problem hiding this comment.
The AffectedFunctions and AffectReasons fields of the Impact struct are now tagged with json:"-", excluding them from the JSON output. However, the README.md JSON output example still shows "affected_functions" as part of the impact object, and SPEC.md documents both affected_functions and affect_reasons with their original JSON tags. These documentation artifacts are now incorrect and should be updated to match the new JSON output, which only includes "services_to_build" under "impact".
No description provided.