Refactor pointer utilities with generic implementations#95
Conversation
|
Warning Rate limit exceeded@AdityaHarindar has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdded generic Go utilities Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant PointerFunc as Pointer[T]
participant DerefFunc as Dereference[T]
Caller->>PointerFunc: Pointer(value)
PointerFunc-->>Caller: *T (non-nil)
Caller->>DerefFunc: Dereference(ptr)
alt ptr != nil
DerefFunc-->>Caller: value (*ptr)
else ptr == nil
Note right of DerefFunc #f6f6f6: returns zero value of T
DerefFunc-->>Caller: zero(T)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Replace type-specific pointer helper functions with generic Pointer[T]
and Dereference[T] implementations that work with any type.
Changes:
- Add generic Pointer[T any](val T) *T function to create pointers
- Add generic Dereference[T any](ptr *T) T function to safely dereference
- Deprecate type-specific functions: BoolP, PBool, StringP, PString,
IntP, PInt, Int32P, PInt32, Int64P, PInt64
- Add comprehensive unit tests with 100% coverage for generic functions
- Test coverage includes primitives, structs, nil handling, and zero values
Signed-off-by: AdityaHarindar <aditya.harindar@gmail.com>
AdityaHarindar
force-pushed
the
refactor/pointer-generics
branch
from
21fa3ec to
78e1b85
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
utils/pointer.go (2)
27-135: Consider adding deprecation timeline.The deprecation comments are clear, but adding a version or timeline for removal would help users plan their migration. For example:
// Deprecated: Use Pointer instead. Will be removed in v2.0.0.
27-135: Plan migration for deprecated pointer utilities
- Found one usage in
db/mongo.go:475(wc.Journal = utils.BoolP(true)); create a migration task to replace allBoolP/PBool/... calls with the newPointer/Dereferencehelpers.- Annotate each deprecated function comment with a removal version (e.g. “will be removed in v2.0.0”) to guide downstream consumers.
utils/pointer_test.go (1)
214-280: Consider adding struct type to round-trip test.While the round-trip test is thorough for primitives, adding a struct test case would make it more complete, matching the coverage in
TestPointerandTestDereference.Add this test case to the
testsslice:{ name: "struct", test: func(t *testing.T) { type testStruct struct { Name string Age int } val := testStruct{Name: "Bob", Age: 25} ptr := Pointer(val) result := Dereference(ptr) if result.Name != val.Name || result.Age != val.Age { t.Errorf("round trip failed: got %v, want %v", result, val) } }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
utils/pointer.go(10 hunks)utils/pointer_test.go(1 hunks)
🔇 Additional comments (2)
utils/pointer.go (1)
6-25: LGTM! Clean generic implementations.Both
PointerandDereferenceare correctly implemented. The nil check inDereferenceproperly returns the zero value for typeT, and the usage examples in the doc comments are helpful.utils/pointer_test.go (1)
11-280: Excellent test coverage for the generic utilities.The test suite is comprehensive and well-structured:
- Covers multiple primitive types (bool, string, int, int32, int64)
- Tests struct types
- Validates nil pointer handling
- Tests zero values
- Includes round-trip verification
All tests follow Go testing best practices with clear subtest names and descriptive error messages.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
utils/pointer.go (1)
27-135: Refactor deprecated functions to delegate to generics.The deprecated functions currently duplicate the implementation logic from
PointerandDereference. This creates a maintenance burden—bug fixes or changes would need to be applied in multiple places.Refactor the deprecated functions to delegate to the generic implementations, ensuring a single source of truth:
// Deprecated: Use Pointer instead. // BoolP returns a pointer to the given bool value. // Usage: // // ptr := utils.BoolP(true) // *bool pointing to true func BoolP(val bool) *bool { - return &val + return Pointer(val) } // Deprecated: Use Dereference instead. // PBool returns the value of a *bool pointer, or false if the pointer is nil. // Usage: // // val := utils.PBool(ptr) // returns value pointed by ptr, or false if ptr is nil func PBool(ptr *bool) bool { - var val bool - if ptr != nil { - val = *ptr - } - return val + return Dereference(ptr) }Apply similar changes to the remaining deprecated functions (StringP, PString, IntP, PInt, Int32P, PInt32, Int64P, PInt64).
🧹 Nitpick comments (1)
utils/pointer_test.go (1)
214-280: Consider adding a struct test case for consistency.The round-trip test covers primitive types (bool, string, int, int32, int64) but omits struct types, which are tested in both
TestPointer(lines 67-80) andTestDereference(lines 188-211). Adding a struct case here would ensure complete consistency across all three test functions.Example addition:
{ name: "int64", test: func(t *testing.T) { val := int64(123) ptr := Pointer(val) result := Dereference(ptr) if result != val { t.Errorf("round trip failed: got %v, want %v", result, val) } }, }, + { + name: "struct", + test: func(t *testing.T) { + type testStruct struct { + Name string + Age int + } + val := testStruct{Name: "Bob", Age: 25} + ptr := Pointer(val) + result := Dereference(ptr) + if result != val { + t.Errorf("round trip failed: got %v, want %v", result, val) + } + }, + }, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
utils/pointer.go(10 hunks)utils/pointer_test.go(1 hunks)
🔇 Additional comments (3)
utils/pointer_test.go (2)
10-99: LGTM! Comprehensive test coverage for Pointer.The test covers multiple types (bool, string, int, int32, int64, struct) and importantly includes zero values. The assertions correctly verify non-nil pointers and accurate dereferencing.
101-212: LGTM! Thorough nil-safety coverage for Dereference.The test properly validates both non-nil pointer dereferencing and nil pointer handling, ensuring zero values are returned. Coverage across primitive types and structs is comprehensive.
utils/pointer.go (1)
6-25: LGTM! Clean generic implementations.Both
Pointer[T any]andDereference[T any]are correctly implemented with clear documentation and usage examples. The generic approach provides type safety across any type while eliminating code duplication.
Signed-off-by: AdityaHarindar <aditya.harindar@gmail.com>
| // | ||
| // ptr := utils.Pointer(42) // *int pointing to 42 | ||
| // ptr := utils.Pointer("hello") // *string pointing to "hello" | ||
| func Pointer[T any](val T) *T { |
There was a problem hiding this comment.
better to change to P for Pointer and V for Dereference
Signed-off-by: AdityaHarindar <aditya.harindar@gmail.com>
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
Caution The MK3 agent failed during execution: Clone operation failed |
Signed-off-by: AdityaHarindar <aditya.harindar@gmail.com>
2 participants
Summary
Pointer[T any]andDereference[T any]functions to replace type-specific implementationsWhat Changed
Pointer[T]andDereference[T]functions that work with any typeBenefits
Test Plan
Summary by CodeRabbit
Chores
Tests