dot notation - #5
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a Graphviz DOT output option to the buildgraph analyze CLI so impact results can be visualized as a call graph diagram.
Changes:
- Extend
analyze --formatto acceptdotin addition totextandjson. - Update output plumbing to pass the call graph into
writeOutputand add aformatDotrenderer. - Add a PNG artifact under
testproject/(presumably an example rendering).
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
cli/analyze.go |
Adds dot to the --format flag help text and passes the call graph into output. |
cli/output.go |
Changes writeOutput signature and implements DOT formatting via formatDot. |
cli/cli_test.go |
Updates tests to match the new writeOutput(result, graph, ...) signature. |
testproject/impact.png |
Adds a rendered PNG asset related to the new visualization output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // formatDot renders the impact as a Graphviz DOT digraph. | ||
| // | ||
| // Layout: | ||
| // - One cluster (subgraph) per service that needs to be rebuilt. | ||
| // - Nodes are short function names; changed functions are filled red, | ||
| // transitively-affected functions are filled orange. | ||
| // - Edges represent caller → callee relationships drawn from the call graph, | ||
| // restricted to nodes that appear in the impact set. |
There was a problem hiding this comment.
The formatDot doc comment says the layout emits “one cluster per service that needs to be rebuilt”, but the implementation iterates over result.Impact.AffectedFunctions owners (which includes non-service owners like core modules). Please align the comment with the actual behavior (cluster per owner, with service owners highlighted when they’re in ServicesToBuild) to avoid misleading users/maintainers.
| // shortLabel strips the module prefix for readability. | ||
| shortLabel := func(fn string) string { | ||
| // Keep only "package.Func" — last two dot-separated segments. | ||
| parts := strings.Split(fn, "/") | ||
| if len(parts) == 0 { | ||
| return fn | ||
| } | ||
| last := parts[len(parts)-1] | ||
| return last |
There was a problem hiding this comment.
shortLabel’s comment says it keeps only the last two dot-separated segments (e.g. “package.Func”), but the code only strips path segments separated by “/” and returns the last path segment unchanged (e.g. “module-a.Process”). Update either the logic or the comment so they match.
| seen := make(map[string]bool) | ||
| for _, fn := range fns { | ||
| if seen[fn] { | ||
| continue | ||
| } | ||
| seen[fn] = true | ||
| id := dotID(fn) | ||
| lbl := shortLabel(fn) | ||
| if changed[fn] { | ||
| fmt.Fprintf(sb, " %s [label=%q, fillcolor=\"#ff6b6b\", fontcolor=white];\n", id, lbl) | ||
| } else { | ||
| fmt.Fprintf(sb, " %s [label=%q, fillcolor=\"#ffd580\"];\n", id, lbl) | ||
| } | ||
| } |
There was a problem hiding this comment.
DOT output is likely nondeterministic: nodes are emitted in the order they appear in the Impact.AffectedFunctions slices (which are built via map iteration in impact.ComputeImpact), so consecutive runs can produce different node ordering within a cluster. Consider sorting the unique function list before emitting nodes to make output stable (helps diffs, caching, and testing).
| fmt.Fprintln(sb, " // edges") | ||
| edgesSeen := make(map[string]bool) | ||
| for fn := range affected { | ||
| node, ok := graph.Nodes[fn] | ||
| if !ok { | ||
| continue | ||
| } | ||
| for _, dep := range node.Deps { | ||
| if !affected[dep.FullName] { | ||
| continue | ||
| } | ||
| key := dotID(fn) + "->" + dotID(dep.FullName) | ||
| if edgesSeen[key] { | ||
| continue | ||
| } | ||
| edgesSeen[key] = true | ||
| fmt.Fprintf(sb, " %s -> %s;\n", dotID(fn), dotID(dep.FullName)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Edge emission order is nondeterministic because it ranges over the affected map; the resulting DOT output can change between runs even when the graph is identical. Consider collecting edges into a slice, sorting, then printing to make the output stable and easier to diff/test.
| func writeOutput(result *types.Result, graph *types.CallGraph, format, outputPath string) { | ||
| var output []byte | ||
| switch format { | ||
| case "text": | ||
| output = []byte(formatText(result)) | ||
| case "dot": | ||
| output = []byte(formatDot(result, graph)) |
There was a problem hiding this comment.
A new output format (“dot” / formatDot) is introduced, but there are no tests exercising it (e.g., ensuring clusters/nodes/edges are emitted and that changed nodes are styled correctly). Since cli/output_test.go already covers text formatting, adding a focused unit test for DOT output would help prevent regressions.
No description provided.