chore(encoding): dropped go-json direct dependency #2033
Conversation
WalkthroughThe pull request introduces changes to the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (6)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (1)
mod/primitives/go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
- mod/primitives/go.mod (0 hunks)
- mod/primitives/pkg/encoding/json/json.go (1 hunks)
💤 Files with no reviewable changes (1)
- mod/primitives/go.mod
🔇 Additional comments (5)
mod/primitives/pkg/encoding/json/json.go (5)
34-34: LGTM! Verify usage in codebase.The change from
gojson.Unmarshalertojson.Unmarshaleris correct and aligns with the PR objective. Both interfaces are identical, so this should not introduce any functional changes.To ensure this change doesn't impact the codebase, run the following script:
#!/bin/bash # Description: Check for any custom implementations of the Unmarshaler interface # that might rely on gojson-specific features. # Test: Search for struct types implementing the Unmarshaler interface rg --type go '(type\s+\w+\s+struct.*\n)*\s*func\s*\(\w+\s*\**\w+\)\s*UnmarshalJSON\s*\(.*\)\s*error'
42-42: LGTM! Verify usage in codebase.The change from
gojson.MarshalIndenttojson.MarshalIndentis correct and aligns with the PR objective. However, there might be slight differences in the output format or performance between the two implementations.To ensure this change doesn't impact the codebase, run the following script:
#!/bin/bash # Description: Check for any direct usage of MarshalIndent that might rely on gojson-specific features or formatting. # Test: Search for direct usage of MarshalIndent function rg --type go '\bMarshalIndent\s*\('Consider reviewing the output of
MarshalIndentin your tests to ensure the indentation format meets your requirements.
46-46: LGTM! Verify usage and consider performance testing.The change from
gojson.Unmarshaltojson.Unmarshalis correct and aligns with the PR objective. However, there might be slight performance differences between the two implementations.To ensure this change doesn't impact the codebase and to assess any performance implications, run the following scripts:
#!/bin/bash # Description: Check for any direct usage of Unmarshal that might rely on gojson-specific features or performance characteristics. # Test: Search for direct usage of Unmarshal function rg --type go '\bUnmarshal\s*\(' # Test: Search for any performance-critical sections using Unmarshal rg --type go -C 10 '(benchmark|performance|critical).*Unmarshal'Consider running performance tests to compare the unmarshaling speed between
gojson.Unmarshalandjson.Unmarshalfor your specific use cases.
38-38: LGTM! Verify usage and consider performance testing.The change from
gojson.Marshaltojson.Marshalis correct and aligns with the PR objective. However, there might be slight performance differences between the two implementations.To ensure this change doesn't impact the codebase and to assess any performance implications, run the following scripts:
Consider running performance tests to compare the marshaling speed between
gojson.Marshalandjson.Marshalfor your specific use cases.✅ Verification successful
Verified the change to
json.Marshal. No performance-critical usages ofMarshalwere found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any direct usage of Marshal that might rely on gojson-specific features or performance characteristics. # Test: Search for direct usage of Marshal function rg --type go '\bMarshal\s*\(' # Test: Search for any performance-critical sections using Marshal rg --type go -C 10 '(benchmark|performance|critical).*Marshal'Length of output: 3297
Script:
#!/bin/bash # Description: Re-check for any direct usage of Marshal and ensure the search term is correct. # Test: Search for direct usage of Marshal function rg --type go '\bMarshal\s*\(' # Test: Search for any performance-critical sections using Marshal rg --type go -C 10 '(benchmark|performance|critical).*Marshal'Length of output: 2088
Script:
#!/bin/bash # Description: Verify if Marshal is used in performance-critical sections with corrected search patterns. # Test: Search for any performance-critical sections using Marshal rg --type go '(benchmark|performance|critical).*Marshal' -C 10Length of output: 64
30-30: LGTM! Verify usage in codebase.The change from
gojson.Marshalertojson.Marshaleris correct and aligns with the PR objective. Both interfaces are identical, so this should not introduce any functional changes.To ensure this change doesn't impact the codebase, run the following script:
✅ Verification successful
LGTM! Usage verified across the codebase. No issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any custom implementations of the Marshaler interface # that might rely on gojson-specific features. # Test: Search for struct types implementing the Marshaler interface rg --type go '(type\s+\w+\s+struct.*\n)*\s*func\s*\(\w+\s*\**\w+\)\s*MarshalJSON\s*\(\)\s*\(.*\)'Length of output: 3297
Script:
#!/bin/bash # Description: Check for any custom implementations of the Marshaler interface # that might rely on gojson-specific features. # Test: Search for struct types implementing the MarshalJSON method rg --type go '(type\s+\w+\s+struct.*\n)*\s*func\s*\(\w+\s*\**\w+\)\s*MarshalJSON\s*\(\)\s*\(.*\)'Length of output: 291
Script:
#!/bin/bash # Description: Check for any custom implementations of the Marshaler interface # that might rely on gojson-specific features. # Test: Search for struct types implementing the MarshalJSON method with multiline support rg -U --type go '(type\s+\w+\s+struct.*\n)*\s*func\s*\(\w+\s*\**\w+\)\s*MarshalJSON\s*\(\)\s*\(.*\)'Length of output: 1467
| type Marshaler = json.Marshaler | ||
|
|
||
| // Unmarshaler is the interface implemented by types | ||
| // that can unmarshal a JSON description of themselves. | ||
| type Unmarshaler = gojson.Unmarshaler | ||
| type Unmarshaler = json.Unmarshaler | ||
|
|
||
| // Marshal is a wrapper for gojson.Marshal, which provides high-performance JSON | ||
| // encoding. | ||
| var Marshal = gojson.Marshal | ||
| var Marshal = json.Marshal | ||
|
|
||
| // MarshalIndent is a wrapper for gojson.MarshalIndent, which provides | ||
| // high-performance JSON encoding with indentation. | ||
| var MarshalIndent = gojson.MarshalIndent | ||
| var MarshalIndent = json.MarshalIndent | ||
|
|
||
| // Unmarshal is a wrapper for gojson.Unmarshal, which provides high-performance | ||
| // JSON decoding. | ||
| var Unmarshal = gojson.Unmarshal | ||
| var Unmarshal = json.Unmarshal |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Overall LGTM! Consider performance impact and thorough testing.
The changes in this file consistently replace gojson with the standard encoding/json package, aligning well with the PR objective of dropping the direct dependency on go-json. All individual changes look good.
Recommendations:
- Run the verification scripts provided in the previous comments to ensure no usage of
gojson-specific features in the codebase. - Conduct thorough testing, especially for any performance-critical sections that use these JSON functions.
- Consider running benchmarks to compare the performance of
encoding/jsonvsgo-jsonfor your specific use cases. - Update any documentation or comments that might reference
go-json.
Given that go-json remains an indirect dependency, consider evaluating if it can be completely removed from the project in the future to further simplify the dependency structure.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2033 +/- ##
=======================================
Coverage 22.42% 22.42%
=======================================
Files 358 358
Lines 16012 16012
Branches 12 12
=======================================
Hits 3591 3591
Misses 12272 12272
Partials 149 149 |
Replaced
go-jsonwithencoding/jsonlibrary.Note that
go-jsonis still an indirect dependency, but only an indirect dependency.Summary by CodeRabbit
New Features
Bug Fixes
gojsonlibrary, streamlining the module's requirements.