Add error returns to walk functions #6
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces error handling capabilities to the radix tree's walk functions by adding error return values to WalkFn, WalkHandler, and the public walk methods (Walk, WalkPrefix, WalkPath). This is a breaking API change that allows walk operations to propagate errors from user-provided handlers.
Key Changes:
- Modified
WalkFnandWalkHandlersignatures to return errors in addition to their existing return values - Updated
Walk,WalkPrefix, andWalkPathmethods to return errors - Modified
recursiveWalkinternal function to propagate errors through the recursion stack - Updated all tests and benchmarks to comply with the new signatures
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| radix.go | Core implementation changes: updated type signatures for WalkFn, WalkHandler, and walk methods to support error propagation; modified recursiveWalk to handle and return errors throughout the tree traversal |
| radix_test.go | Comprehensive test updates: modified all walk function signatures to match new API, added TestWalkErrors to verify error propagation works correctly for Walk/WalkPrefix/WalkPath methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| return WalkContinue, zero | ||
| return WalkContinue, zero, nil | ||
| } | ||
| t.Walk(fn) |
There was a problem hiding this comment.
The error returned by t.Walk(fn) is not being handled. Since Walk now returns an error, it should be checked and handled appropriately. Consider either returning the error from ToMap by changing its signature to func (t *Tree[T]) ToMap() (map[string]T, error), or silently ignoring it if errors are not expected here. Given that the walk function used here always returns nil, this might be acceptable, but the API design should be consistent.
| var fn WalkFn[int] = func(s string, v int) (WalkFlag, int, error) { | ||
| return WalkContinue, v, nil | ||
| } | ||
| r.Walk(fn) |
There was a problem hiding this comment.
The error returned by r.Walk(fn) is not being checked. Since Walk now returns an error, it should be checked in tests to ensure no unexpected errors occur. Consider using c.Assert(r.Walk(fn), qt.IsNil) or similar.
| r.Walk(fn) | |
| c.Assert(r.Walk(fn), qt.IsNil) |
| return WalkSet, v * 10, nil | ||
| }, | ||
| } | ||
| r.Walk(h) |
There was a problem hiding this comment.
The error returned by r.Walk(h) is not being checked. Since Walk now returns an error, it should be checked in tests to ensure no unexpected errors occur.
| var fn WalkFn[int] = func(s string, v int) (WalkFlag, int, error) { | ||
| return WalkSet, v * 3, nil | ||
| } | ||
| r.Walk(fn) |
There was a problem hiding this comment.
The error returned by r.Walk(fn) is not being checked. Since Walk now returns an error, it should be checked in tests to ensure no unexpected errors occur.
| r.Walk(fn) | |
| err := r.Walk(fn) | |
| c.Assert(err, qt.IsNil) |
| return WalkContinue, "" | ||
| return WalkContinue, "", nil | ||
| } | ||
| r.WalkPrefix(test.inp, fn) |
There was a problem hiding this comment.
The error returned by r.WalkPrefix(test.inp, fn) is not being checked. Since WalkPrefix now returns an error, it should be checked in tests to ensure no unexpected errors occur.
| r.WalkPrefix(test.inp, fn) | |
| err := r.WalkPrefix(test.inp, fn) | |
| c.Assert(err, qt.IsNil) |
| return WalkSet, v * 10, nil | ||
| }, | ||
| } | ||
| r.Walk(h) |
There was a problem hiding this comment.
The error returned by r.Walk(h) is not being checked. Since Walk now returns an error, it should be checked in tests to ensure no unexpected errors occur.
| return WalkSet, v * 10, nil | ||
| }, | ||
| } | ||
| r.Walk(h) |
There was a problem hiding this comment.
The error returned by r.Walk(h) is not being checked. Since Walk now returns an error, it should be checked in tests to ensure no unexpected errors occur.
| r.Walk(h) | |
| err := r.Walk(h) | |
| c.Assert(err, qt.IsNil) |
| r.Walk(fn) | ||
|
|
There was a problem hiding this comment.
The error returned by r.Walk(fn) is not being checked. Since Walk now returns an error, it should be checked in tests to ensure no unexpected errors occur.
| r.Walk(fn) | |
| err := r.Walk(fn) | |
| c.Assert(err, qt.IsNil) |
| return WalkContinue, false | ||
| return WalkContinue, false, nil | ||
| } | ||
| r.Walk(fn) |
There was a problem hiding this comment.
The error returned by r.Walk(fn) is not being checked. Since Walk now returns an error, it should be checked in tests to ensure no unexpected errors occur.
| r.Walk(fn) | |
| err := r.Walk(fn) | |
| c.Assert(err, qt.IsNil) |
| return WalkContinue, "" | ||
| return WalkContinue, "", nil | ||
| } | ||
| r.WalkPath(test.inp, fn) |
There was a problem hiding this comment.
The error returned by r.WalkPath(test.inp, fn) is not being checked. Since WalkPath now returns an error, it should be checked in tests to ensure no unexpected errors occur.
| r.WalkPath(test.inp, fn) | |
| err := r.WalkPath(test.inp, fn) | |
| c.Assert(err, qt.IsNil) |
1 participant
No description provided.