chore(primitives): Dropped hex.String
#2048
Conversation
WalkthroughThe changes in this pull request introduce new functionalities for encoding and decoding big integers as hexadecimal strings in the Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2048 +/- ##
==========================================
- Coverage 23.50% 23.39% -0.12%
==========================================
Files 356 355 -1
Lines 16054 16025 -29
Branches 12 12
==========================================
- Hits 3774 3749 -25
+ Misses 12115 12111 -4
Partials 165 165
|
abi87
changed the title
hex.Stringhex.String
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range comments (1)
mod/primitives/pkg/encoding/hex/u64_test.go (1)
Line range hint
57-77: ImprovedTestValidateQuotedStringfunctionThe changes to this test function are well-implemented:
- The function has been correctly renamed to
TestValidateQuotedString, reflecting the changes in the tested function.- The function now calls
hex.ValidateQuotedString, which is consistent with the renaming.- Error handling has been simplified, improving readability.
These changes align well with the PR objectives of streamlining the code.
However, a minor improvement could be made to the error assertion:
Consider using
require.ErrorIsinstead ofrequire.Equalfor error comparison. This change would make the test more robust against potential wrapping of errors in the future. Here's the suggested change:- require.Equal(t, test.expected, err) + require.ErrorIs(t, err, test.expected)This change would apply to line 73 of the provided code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (13)
- mod/primitives/pkg/bytes/b.go (1 hunks)
- mod/primitives/pkg/bytes/b_test.go (2 hunks)
- mod/primitives/pkg/encoding/hex/big_int.go (1 hunks)
- mod/primitives/pkg/encoding/hex/bit_int_test.go (1 hunks)
- mod/primitives/pkg/encoding/hex/bytes.go (1 hunks)
- mod/primitives/pkg/encoding/hex/format.go (1 hunks)
- mod/primitives/pkg/encoding/hex/format_test.go (1 hunks)
- mod/primitives/pkg/encoding/hex/hex_test.go (0 hunks)
- mod/primitives/pkg/encoding/hex/string.go (0 hunks)
- mod/primitives/pkg/encoding/hex/u64.go (0 hunks)
- mod/primitives/pkg/encoding/hex/u64_test.go (3 hunks)
- mod/primitives/pkg/math/u64.go (1 hunks)
- mod/primitives/pkg/math/u64_test.go (1 hunks)
💤 Files with no reviewable changes (3)
- mod/primitives/pkg/encoding/hex/hex_test.go
- mod/primitives/pkg/encoding/hex/string.go
- mod/primitives/pkg/encoding/hex/u64.go
🧰 Additional context used
🔇 Additional comments (18)
mod/primitives/pkg/bytes/b.go (1)
39-43: Improved input validation and consistency in UnmarshalJSONThe changes to the
UnmarshalJSONmethod are well-structured and improve the overall robustness of the code. The addition of input validation usinghex.ValidateQuotedStringis a good practice. The use ofb.UnmarshalTextpromotes code reuse and aligns with Go's standard library approach.To ensure full compatibility, let's verify the behavior of
hex.ValidateQuotedString:✅ Verification successful
Validation of
hex.ValidateQuotedStringConfirmedThe
ValidateQuotedStringfunction is properly implemented and has corresponding test coverage, ensuring that input validation behaves as expected. This verifies the correctness and robustness of the changes made to theUnmarshalJSONmethod.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of hex.ValidateQuotedString # Test: Check the implementation of ValidateQuotedString ast-grep --lang go --pattern 'func ValidateQuotedString(input []byte) ([]byte, error) { $$$ }' # Test: Check for any tests related to ValidateQuotedString rg --type go -i 'func TestValidateQuotedString'Length of output: 712
mod/primitives/pkg/encoding/hex/format_test.go (2)
21-21: Approved: Package name change enhances test isolationChanging the package name from
hextohex_testis a good practice. This change ensures that the tests are using the public API of thehexpackage and prevents test code from being included in the main package.
Line range hint
1-71: Verify the removal ofUnmarshalJSONTextfunctionThe AI summary mentions the removal of the
UnmarshalJSONTextfunction, but this change is not visible in the provided diff. It's important to verify that this removal doesn't negatively impact the package's functionality.Please run the following script to check for any remaining references to
UnmarshalJSONText:If this search returns any results, we should review those occurrences to ensure they've been properly addressed or removed.
✅ Verification successful
Verified the removal of
UnmarshalJSONTextfrom thehexpackage.
There are no remaining references toUnmarshalJSONTextin thepkg/encoding/hexdirectory.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to UnmarshalJSONText in the hex package # Test: Search for UnmarshalJSONText in the hex package rg --type go "UnmarshalJSONText" pkg/encoding/hexLength of output: 106
Script:
#!/bin/bash # Description: Search the entire repository for any references to UnmarshalJSONText # Search for UnmarshalJSONText in all Go files within the repository rg --type go "UnmarshalJSONText"Length of output: 115
mod/primitives/pkg/encoding/hex/big_int.go (3)
1-23: LGTM: License, package declaration, and import are correct.The license header, package declaration, and import statement are all appropriate and correctly implemented.
73-81: LGTM: MustToBigInt function is correctly implemented.The MustToBigInt function is a well-implemented wrapper around ToBigInt. It correctly panics on error and provides a clear, documented API for cases where the caller wants to avoid error checking.
1-81: Overall, the file is well-implemented and aligns with PR objectives.This new file successfully introduces the functionality for encoding and decoding big integers as hexadecimal strings, as described in the AI-generated summary. The implementation is generally robust, with proper error handling and clear function signatures.
The changes align well with the PR objectives of streamlining the hex package by introducing standalone functions for BigInt operations that were previously part of the removed
hex.Stringtype.A few minor improvements were suggested:
- Clarifying the precondition for the
FromBigIntfunction.- A potential optimization in the
ToBigIntfunction for common cases.These changes will further enhance the clarity and efficiency of the code.
mod/primitives/pkg/encoding/hex/u64_test.go (2)
34-41: Improved test cases forTestMarshalTextThe changes to the
TestMarshalTextfunction are well-implemented:
- The expected output type has been correctly changed from
stringto[]byte, which is more consistent with the typical return type ofMarshalTextmethods in Go.- A new test case for a positive value (12345) has been added, improving the test coverage by including a non-zero, non-max value.
These changes enhance the robustness and completeness of the test suite.
44-52: Enhanced test coverage inTestMarshalTextThe changes to the test execution logic in
TestMarshalTextare well-implemented:
- The assertion has been correctly updated to use the new
[]byteexpected type, maintaining consistency with the test case changes.- A decoding step has been added (lines 50-52), which verifies the roundtrip consistency of the marshaling and unmarshaling process. This significantly improves the test coverage by ensuring that the encoded value can be correctly decoded back to its original form.
These changes enhance the overall quality and reliability of the test suite.
mod/primitives/pkg/encoding/hex/bit_int_test.go (3)
1-22: LGTM: License header and package declaration are correct.The BUSL-1.1 license header is properly included, and the package is correctly declared as
hex_test, following Go conventions for test files.
24-31: LGTM: Import statements are appropriate.The import block includes all necessary packages, including standard library packages and the project's
hexpackage. The use ofrequirefrom the testify package is a good practice for concise and clear test assertions.
1-110: Overall, excellent test coverage and structure.This new test file provides comprehensive coverage for the
hexpackage'sBigIntrelated functions. The tests are well-structured, use table-driven testing, and cover important scenarios including edge cases. The minor suggestions for improvement (adding a negative number test case and a very large number test case) would further enhance the robustness of the test suite.Great job on writing thorough and maintainable tests!
mod/primitives/pkg/encoding/hex/bytes.go (1)
Line range hint
1-81: Overall, the changes improve input validation without disrupting the file structureThe modifications to
DecodeFixedJSONenhance the input validation process while maintaining the overall structure and functionality of the file. The rest of the file, including other utility functions for hexadecimal encoding and decoding, remains unchanged and well-organized.mod/primitives/pkg/math/u64_test.go (1)
63-66: Excellent addition of new test cases for improved coverage!The new test cases for "Invalid JSON text", "Invalid quoted JSON text", and "Empty JSON text" significantly enhance the robustness of the
TestU64_UnmarshalJSONfunction. These additions cover important edge cases that were previously untested, ensuring that theUnmarshalJSONmethod handles various error scenarios correctly.
- The "Invalid JSON text" case checks the behavior when an empty string is provided.
- The "Invalid quoted JSON text" case verifies the handling of an incomplete JSON string.
- The "Empty JSON text" case tests the response to an empty quoted string.
These improvements will help catch potential issues related to JSON unmarshalling, making the code more reliable and maintainable.
mod/primitives/pkg/bytes/b_test.go (2)
Line range hint
139-917: Summary: Improved test coverage and naming clarity.The changes in this file enhance the test suite by:
- Adding a new test function
TestBytesUnmarshalJSONTextto cover JSON unmarshalling scenarios.- Renaming
TestUnmarshalJSONtoTestBytes48UnmarshalJSONfor better specificity.These modifications improve the overall quality and maintainability of the test suite.
917-917: LGTM! Verify no unintended changes in the function body.The renaming of
TestUnmarshalJSONtoTestBytes48UnmarshalJSONimproves clarity by specifying the exact type being tested. This change aligns well with the naming convention of other test functions in the file.Please run the following script to verify that no unintended changes were made to the function body:
mod/primitives/pkg/encoding/hex/format.go (3)
23-26: Necessary imports addedThe addition of the
"strings"package is appropriate for the new string manipulation functions.
31-42:IsValidHexfunction enhances hex validationThe new
IsValidHexfunction provides a robust and generic approach to hex string validation, effectively handling both[]byteandstringtypes. The use of type approximation~[]byte | ~stringaligns with Go's best practices for generics.
44-50:ValidateQuotedStringfunction correctly validates quoted stringsThe
ValidateQuotedStringfunction efficiently checks for properly quoted input and returns the unquoted content, enhancing input validation processes.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- mod/primitives/pkg/encoding/hex/u64_test.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (1)
mod/primitives/pkg/encoding/hex/u64_test.go (1)
34-34: Test cases updated to use[]bytefor expected valuesThe test cases in
TestMarshalTexthave been correctly updated to use[]bytefor theexpectedfield, matching the change in the expected output type. This ensures that the tests properly validate the function's output.Also applies to: 36-41
abi87
changed the title
hex.Stringhex.String
| } | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
q: in UnmarshalUint64Text, I wonder if we should use return strconv.ParseUint(string(raw), hexBase, 0) instead of the current implementation?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range comments (1)
mod/primitives/pkg/encoding/hex/format.go (1)
Line range hint
61-70: Ensure consistent type constraints informatAndValidateNumberThe type constraint for
TinformatAndValidateNumberis[]byte | string, whereasIsValidHexuses~[]byte | ~string. For consistency and to support types derived from[]byteorstring, consider modifying the type constraint.Update the function signature to:
-func formatAndValidateNumber[T []byte | string](input T) (T, error) { +func formatAndValidateNumber[T ~[]byte | ~string](input T) (T, error) {This change aligns the type constraints and ensures that both functions handle the same set of types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (4)
- mod/primitives/pkg/encoding/hex/big_int.go (1 hunks)
- mod/primitives/pkg/encoding/hex/bytes.go (1 hunks)
- mod/primitives/pkg/encoding/hex/format.go (1 hunks)
- mod/primitives/pkg/encoding/hex/u64.go (0 hunks)
💤 Files with no reviewable changes (1)
- mod/primitives/pkg/encoding/hex/u64.go
🧰 Additional context used
🔇 Additional comments (6)
mod/primitives/pkg/encoding/hex/bytes.go (2)
77-81: Add comprehensive tests for ValidateQuotedString in bytes.goWhile
ValidateQuotedStringis implemented informat.goand has tests inu64_test.go, there are no tests covering its usage inbytes.go. To ensure robust validation and prevent potential issues, please add relevant tests inbytes_test.goto cover all edge cases and usage scenarios withinbytes.go.
77-81: Improved input validation, but verify compatibility and error handlingThe changes to
DecodeFixedJSONenhance input validation by introducingValidateQuotedString. This likely provides more robust checking than the previousisQuotedStringmethod. The error handling is also more specific, which could aid in debugging.However, please consider the following points:
- Ensure that
ValidateQuotedStringis thoroughly tested and handles all edge cases.- Verify that the change in error handling doesn't negatively impact any downstream error processing.
- Confirm that removing the explicit check for quoted strings doesn't break backwards compatibility with any existing clients.
To verify these points, run the following script:
✅ Verification successful
Validation of
ValidateQuotedStringimplementation and usage confirmedThe
ValidateQuotedStringfunction is properly implemented and utilized across multiple modules. Comprehensive tests are in place to ensure its reliability. Additionally, the removal ofisQuotedStringdoes not appear to impact backward compatibility.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify ValidateQuotedString implementation, usage, and tests # Test 1: Find the implementation of ValidateQuotedString echo "Searching for ValidateQuotedString implementation:" rg --type go "func ValidateQuotedString" # Test 2: Check for other usages of ValidateQuotedString echo "Searching for other usages of ValidateQuotedString:" rg --type go "ValidateQuotedString\(" # Test 3: Look for any tests related to ValidateQuotedString echo "Searching for tests related to ValidateQuotedString:" rg --type go "Test.*ValidateQuotedString" # Test 4: Check for potential backwards compatibility issues echo "Searching for potential backwards compatibility issues:" rg --type go "isQuotedString"Length of output: 1353
mod/primitives/pkg/encoding/hex/big_int.go (2)
39-71: LGTM:ToBigIntfunction is well-implemented.The
ToBigIntfunction correctly decodes a hexadecimal string into a*big.Intwith proper input validation and error handling.
73-81: LGTM:MustToBigIntfunction is correctly implemented.The
MustToBigIntfunction effectively handles conversion by panicking on invalid inputs, ensuring that callers receive a valid*big.Intor an immediate failure.mod/primitives/pkg/encoding/hex/format.go (2)
23-26: Approved: Import statements correctly updatedThe import of
"strings"is necessary for the new functions and is appropriately added alongside"errors".
44-50: Approved: New functionValidateQuotedStringadded correctlyThe
ValidateQuotedStringfunction correctly checks for quoted strings and returns the unquoted content, enhancing input validation.
Dropped
hex.Stringtype with the following steps:json.gofile fromhexpackage. Moved UTs to client packages.To/FromUInt64Stringattributes. Only the counterparts inu64.gofile are really used. Moved relevant UTs.BigIntmethods out ofhex.Stringinto their own function.hex.NewStringctor andhex.Stringtype as a wholeThe somewhat large number of lines changed is consequence of the reshuffling of tests to different files.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests