Use json.Number in comparisons, add support for uint64 #25
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #25 +/- ##
===========================================
- Coverage 85.00% 27.34% -57.66%
===========================================
Files 4 9 +5
Lines 280 1024 +744
===========================================
+ Hits 238 280 +42
- Misses 23 724 +701
- Partials 19 20 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR introduces a breaking change in variable type handling, a critical error formatting bug, and API incompatibility in formatter configuration that could disrupt existing integrations.
📄 Documentation Diagram
This diagram documents the refactored JSON comparison workflow with new number handling support.
sequenceDiagram
participant User
participant Comparer
participant Diff
participant Formatter
User->>Comparer: Compare(expected, actual)
Comparer->>Comparer: Filter variables and ignore diffs
note over Comparer: PR #35;25: Added json.Number and uint64 support
Comparer->>Diff: CompareObjects/Arrays
Diff->>Formatter: Format differences
Formatter->>User: Return formatted diff or nil
🌟 Strengths
- Adds support for uint64 and json.Number, improving number handling accuracy.
- Enhances error checking robustness in JSON5 parsing.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P1 | compare.go | Bug | Breaks tests with float64 to int64 type change | path:equal_test.go, symbol:TestComparer_Equal_vars_scalar |
| P1 | compare.go | Bug | Malformed error messages from incorrect formatting | |
| P1 | equal.go | Architecture | API breakage in FormatterConfig type change | symbol:Comparer |
| P2 | diff/diff.go | Bug | Inconsistent number handling in Differ.Compare | |
| P2 | equal_test.go | Testing | Tests confirm type change, may affect users | symbol:TestComparer_Equal_vars_scalar |
| P2 | go.mod | Maintainability | Dependency update for new functionality | |
| P2 | compare.go | Bug | Potential diff truncation in reduceDiff | |
| P2 | json5/json5.go | Maintainability | Improved error checking with errors.Is |
🔍 Notable Themes
- Type System Changes: Shifts from float64 to int64 and json.Number affect variable collection and comparisons, risking backward compatibility.
- Error Handling Consistency: Mix of improvements (e.g., errors.Is) and regressions (malformed errors) highlights need for uniform error management.
- API Contract Risks: FormatterConfig type change breaks public API, requiring user updates for configuration.
📈 Risk Diagram
This diagram illustrates the risks in variable type changes and error handling inconsistencies.
sequenceDiagram
participant Test
participant Comparer
participant Vars
Test->>Comparer: Compare with variable "$var"
Comparer->>Comparer: Collect variable value
note over Comparer: R1(P1): Type change from float64 to int64
Comparer->>Vars: Set variable with new type
Vars-->>Test: Variable stored
Test->>Test: Assertion may fail due to type mismatch
Test->>Comparer: Trigger error case
Comparer->>Comparer: Format error message
note over Comparer: R2(P1): Malformed error with %wv
Comparer-->>Test: Incorrect error output
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| if n, ok := v.(json.Number); ok { | ||
| v = shared.DecodeJSONNumber(n) | ||
| } else if f, ok := v.(float64); ok && f == float64(int64(f)) { | ||
| v = int64(f) | ||
| } | ||
|
|
There was a problem hiding this comment.
P1 | Confidence: High
The variable collection logic now handles json.Number and converts float64 to int64 when possible. This changes the type of collected variables from float64 to int64 for integer values, as seen in TestComparer_Equal_vars_scalar where 123.0 becomes int64(123). This could break existing tests that rely on float64 equality comparisons.
| err = unmarshal(expected, &expDecoded) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to unmarshal expected:\n%wv", err) | ||
| } |
There was a problem hiding this comment.
P1 | Confidence: High
Error formatting uses incorrect verb %wv which will output literal 'v' after wrapped error. This creates malformed error messages like "failed to unmarshal expected: v". Should use %w without trailing 'v'.
| err = unmarshal(expected, &expDecoded) | |
| if err != nil { | |
| return fmt.Errorf("failed to unmarshal expected:\n%wv", err) | |
| } | |
| return fmt.Errorf("failed to unmarshal expected:\n%w", err) |
| // FormatterConfig controls diff formatter configuration. | ||
| FormatterConfig formatter.AsciiFormatterConfig | ||
| FormatterConfig diff.ASCIIFormatterConfig |
There was a problem hiding this comment.
P1 | Confidence: High
The FormatterConfig type changed from formatter.AsciiFormatterConfig to diff.ASCIIFormatterConfig. This breaks compatibility with existing code that configures the formatter. The related_context shows this is part of the public Comparer API.
|
|
||
| assert.True(t, found) | ||
| assert.Equal(t, []interface{}{123.0}, b) | ||
| assert.Equal(t, []interface{}{int64(123)}, b) |
There was a problem hiding this comment.
P2 | Confidence: High
Tests now expect int64 instead of float64 for integer values. This confirms the behavior change in number handling. While the tests pass, external users might be affected by this type change in collected variables.
| require ( | ||
| github.com/bool64/dev v0.2.43 | ||
| github.com/bool64/shared v0.1.5 | ||
| github.com/bool64/shared v0.1.6 |
There was a problem hiding this comment.
P2 | Confidence: High
The shared dependency update to v0.1.6 is required for DecodeJSONNumber functionality. Ensure this version is stable and properly supports uint64 decoding as used in the new comparison logic.
| if !errors.Is(err, io.EOF) { | ||
| return errors.New("unexpected bytes after JSON5 payload") | ||
| } |
There was a problem hiding this comment.
P2 | Confidence: High
The change to errors.Is improves error checking robustness by handling wrapped errors. This is a positive maintainability improvement.
| err := json.Unmarshal(left, &leftMap) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
P2 | Confidence: Medium
The Differ.Compare method unmarshals JSON without UseNumber, while the main comparison logic in compare.go uses UseNumber. This inconsistency could lead to different number handling between direct Differ usage and the main Comparer.
| for k := start; k < end; k++ { | ||
| result = append(result, diffRows[k]) | ||
| } |
There was a problem hiding this comment.
P2 | Confidence: Medium
The reduceDiff logic uses exclusive end bound (k < end) while the old code used inclusive (k <= end). This might truncate the last line of diff context. The behavior should be verified against the original implementation.
2 participants
Hard forks
diffpackage to add needed support and fix lint.