Add cfgx diff command #6
Merged
Conversation
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
📊 Code Coverage ReportCoverage by file |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a new diff command for cfgx that compares two TOML configuration files and highlights their differences. This feature helps developers understand configuration changes between different environments (dev vs prod) or between base and override configurations.
Key changes:
- Added
cfgx diffcommand with text and JSON output formats - Supports recursive comparison of nested TOML tables
- Provides
--keys-onlyand--formatflags for customized output
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/cfgx/diff.go | New file implementing the diff command with comparison logic, output formatting, and CLI interface |
| cmd/cfgx/main.go | Registers the new diff command with the root command |
| readme.md | Adds documentation for the diff command including usage examples and output format |
| example/diff/config.dev.toml | Example development configuration file for demonstrating diff functionality |
| example/diff/config.prod.toml | Example production configuration file for demonstrating diff functionality |
| ROADMAP.md | Removes diff from the roadmap since it's now implemented |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if isMap1 && isMap2 { | ||
| // Recursively compare nested maps | ||
| nestedDiffs := computeDiffs(map1, map2, fullKey) | ||
| diffs = append(diffs, nestedDiffs...) |
There was a problem hiding this comment.
When one value is a map and the other is not (isMap1 != isMap2), the comparison logic falls through without reporting a difference. This results in a silent failure to detect type mismatches. Add an explicit check for this case before the deepEqual comparison.
Suggested change
| diffs = append(diffs, nestedDiffs...) | |
| diffs = append(diffs, nestedDiffs...) | |
| } else if isMap1 != isMap2 { | |
| // Type mismatch: one is a map, the other is not | |
| diffs = append(diffs, Diff{ | |
| Key: fullKey, | |
| Type: DiffChanged, | |
| Value1: val1, | |
| Value2: val2, | |
| }) |
Comment on lines
+178
to
+182
| func deepEqual(v1, v2 any) bool { | ||
| // Use fmt.Sprintf to compare values as strings | ||
| // This handles most TOML types correctly | ||
| return fmt.Sprintf("%v", v1) == fmt.Sprintf("%v", v2) | ||
| } |
There was a problem hiding this comment.
Using fmt.Sprintf for deep equality comparison can produce false positives. For example, arrays and maps with different internal representations may produce the same string output. Consider using reflect.DeepEqual or implementing proper type-specific comparisons for arrays, maps, and primitive types.
Comment on lines
+134
to
+148
| diffs = append(diffs, Diff{ | ||
| Key: fullKey, | ||
| Type: DiffAdded, | ||
| Value2: val2, | ||
| }) | ||
| continue | ||
| } | ||
|
|
||
| // Key only in data1 (removed) | ||
| if exists1 && !exists2 { | ||
| diffs = append(diffs, Diff{ | ||
| Key: fullKey, | ||
| Type: DiffRemoved, | ||
| Value1: val1, | ||
| }) |
There was a problem hiding this comment.
When a key exists only in data2 and its value is a nested map, the entire map is stored in Value2 without recursing into it. This means added nested structures are not expanded in the diff output, making it difficult to see what was added in deeply nested configurations. Consider recursing into added/removed maps to provide detailed diffs.
Suggested change
| diffs = append(diffs, Diff{ | |
| Key: fullKey, | |
| Type: DiffAdded, | |
| Value2: val2, | |
| }) | |
| continue | |
| } | |
| // Key only in data1 (removed) | |
| if exists1 && !exists2 { | |
| diffs = append(diffs, Diff{ | |
| Key: fullKey, | |
| Type: DiffRemoved, | |
| Value1: val1, | |
| }) | |
| // If the added value is a map, recurse into it for detailed diffs | |
| if map2, isMap2 := val2.(map[string]any); isMap2 { | |
| nestedDiffs := computeDiffs(nil, map2, fullKey) | |
| diffs = append(diffs, nestedDiffs...) | |
| } else { | |
| diffs = append(diffs, Diff{ | |
| Key: fullKey, | |
| Type: DiffAdded, | |
| Value2: val2, | |
| }) | |
| } | |
| continue | |
| } | |
| // Key only in data1 (removed) | |
| if exists1 && !exists2 { | |
| // If the removed value is a map, recurse into it for detailed diffs | |
| if map1, isMap1 := val1.(map[string]any); isMap1 { | |
| nestedDiffs := computeDiffs(map1, nil, fullKey) | |
| diffs = append(diffs, nestedDiffs...) | |
| } else { | |
| diffs = append(diffs, Diff{ | |
| Key: fullKey, | |
| Type: DiffRemoved, | |
| Value1: val1, | |
| }) | |
| } |
| - Exits with code 0 on successful comparison (regardless of differences found) | ||
| - Exits with code 1 only on errors (file not found, invalid TOML, etc.) | ||
| - Recursively compares nested tables | ||
| - Shows added (+), removed (-), and changed (~) values |
There was a problem hiding this comment.
The documentation states that changed values are shown with '', but the actual text output in lines 199-202 of diff.go does not prefix changed values with '' unless --keys-only is used. The documentation should accurately reflect the actual behavior where changed values show the key without a prefix, followed by '-' and '+' lines for the old and new values.
Suggested change
| - Shows added (+), removed (-), and changed (~) values | |
| - Shows added (+), removed (-), and changed values (key shown, with '-' and '+' lines for old and new values) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Add
cfgx diffcommand for comparing TOML filesImplements a new
diffcommand that compares two TOML configuration files and highlights differences. This is useful for understanding changes between environments (dev vs prod) or base and override configurations.Features:
--keys-onlyflag to show only changed keys--format jsonfor machine-readable outputExample usage:
Includes example config files and updated documentation.