#13 - Moving order-rely methods to Linear interface#23
Conversation
…nterface; added some docblocks.
WalkthroughThis pull request removes several legacy test functions and their case generators for reverse collection operations, while simultaneously restructuring interface definitions. The Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test Function
participant B as Collection Builder
participant L as Linear Collection
participant M as Reverse Method
T->>B: Build test collection
B->>L: Instantiate linear collection
T->>L: Invoke reverse method (e.g., EachRev, FindLast)
L->>M: Process reverse operation
M-->>L: Return computed results
L-->>T: Deliver output for verification
sequenceDiagram
participant F as comfyFindLast Function
participant L as Linear Collection
participant P as Predicate
F->>L: Pass collection (Linear[V])
L->>P: Evaluate each element against predicate
P-->>L: Return boolean outcome
L-->>F: Provide last matching value or default
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24) Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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 ReportAll modified and coverable lines are covered by tests ✅
🚀 New features to boost your workflow:
|
![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: CHILL
Plan: Pro
📒 Files selected for processing (6)
base_cases_test.go(0 hunks)definitions.go(3 hunks)functions_internal.go(1 hunks)linear_cases_test.go(2 hunks)sequence_test.go(4 hunks)sequencecmp_test.go(4 hunks)
💤 Files with no reviewable changes (1)
- base_cases_test.go
🧰 Additional context used
🪛 GitHub Check: test-and-lint
definitions.go
[failure] 48-48:
File is not properly formatted (gofumpt)
🪛 GitHub Actions: Pull Request Checks
definitions.go
[error] 1-1: File is not properly formatted (gofumpt)
🔇 Additional comments (33)
functions_internal.go (1)
86-104: Properly updated type constraint fromBase[V]toLinear[V]The function signature change correctly updates the type constraint to
Linear[V], reflecting that reverse collection operations now belong to theLinearinterface rather thanBase. This is consistent with the interface restructuring where order-dependent operations are moved to theLinearinterface.sequence_test.go (6)
173-173: Updated type parameter to match the new interface requirementThe change from
baseInternal[int]tolinearInternal[int]correctly reflects thatEachRevnow requires collections to implement theLinearinterface.
177-177: Updated type parameter to match the new interface requirementThe change from
baseInternal[int]tolinearInternal[int]correctly reflects thatEachRevUntilnow requires collections to implement theLinearinterface.
190-191: Updated type parameters forFindLasttest casesThe type parameter changes from
baseInternaltolinearInternalcorrectly reflect thatFindLastoperations now require collections to implement theLinearinterface rather than justBase.
199-199: Updated type parameter to match the new interface requirementThe change from
baseInternal[int]tolinearInternal[int]correctly reflects thatFoldRevnow requires collections to implement theLinearinterface.
232-232: Updated type parameter to match the new interface requirementThe change from
baseInternal[int]tolinearInternal[int]correctly reflects thatReduceRevnow requires collections to implement theLinearinterface.
254-255: Updated type parameters forSearchRevtest casesThe type parameter changes from
baseInternaltolinearInternalcorrectly reflect thatSearchRevoperations now require collections to implement theLinearinterface rather than justBase.sequencecmp_test.go (6)
179-179: Updated type parameter to match the new interface requirementThe change from
baseInternal[int]tolinearInternal[int]correctly reflects thatEachRevnow requires collections to implement theLinearinterface.
183-183: Updated type parameter to match the new interface requirementThe change from
baseInternal[int]tolinearInternal[int]correctly reflects thatEachRevUntilnow requires collections to implement theLinearinterface.
195-195: Updated type parameter to match the new interface requirementThe change from
baseInternal[int]tolinearInternal[int]correctly reflects thatFindLastnow requires collections to implement theLinearinterface.
203-203: Updated type parameter to match the new interface requirementThe change from
baseInternal[int]tolinearInternal[int]correctly reflects thatFoldRevnow requires collections to implement theLinearinterface.
290-290: Updated type parameter to match the new interface requirementThe change from
baseInternal[int]tolinearInternal[int]correctly reflects thatReduceRevnow requires collections to implement theLinearinterface.
315-315: Updated type parameter to match the new interface requirementThe change from
baseInternal[int]tolinearInternal[int]correctly reflects thatSearchRevnow requires collections to implement theLinearinterface.linear_cases_test.go (17)
12-14: Well-defined types for linear collection test cases with pairsThese type definitions create a proper foundation for testing linear collections of pairs, consistent with the restructuring of the interface hierarchy.
16-78: Comprehensive test cases forEachRevoperationThe implementation provides thorough test coverage for the
EachRevoperation, including empty collections, single-item collections, and multi-item collections. The test cases verify both the indices and values passed to the visitor function.
80-93: Robust test function forEachRevoperationThis test function properly executes the test cases for the
EachRevoperation, validating the expected behavior for various collection states.
95-187: Comprehensive test cases forEachRevUntiloperationThe implementation covers all important scenarios for
EachRevUntil, including early termination in the middle of a collection. The test cases are well-designed to verify the correct behavior of the operation.
189-202: Robust test function forEachRevUntiloperationThis test function properly executes the test cases for the
EachRevUntiloperation, validating the expected behavior for various collection states.
204-263: Comprehensive test cases forFindLastoperationThe test cases thoroughly cover the
FindLastoperation for different collection sizes and predicate conditions, ensuring robust testing of the functionality.
265-275: Robust test function forFindLastoperationThis test function properly executes the test cases for the
FindLastoperation, validating the expected behavior for various scenarios.
277-307: Well-designed test cases forFindLastwith duplicate valuesThese test cases specifically target testing
FindLastwith collections containing duplicate values, ensuring that the operation correctly identifies the last matching element in a collection.
309-319: Robust test function forFindLastwith duplicate valuesThis test function properly executes the test cases for the
FindLastoperation with duplicate values, validating the expected behavior in such scenarios.
321-368: Comprehensive test cases forFoldRevoperationThe test cases cover various scenarios for the
FoldRevoperation, including empty collections, single-item collections, and multi-item collections, with different reducer functions.
370-380: Robust test function forFoldRevoperationThis test function properly executes the test cases for the
FoldRevoperation, validating the expected behavior for various collection states.
455-582: Comprehensive test cases forSearchRevoperationThe test cases thoroughly cover the
SearchRevoperation for different collection sizes and search conditions, including the handling of duplicates.
584-600: Robust test function forSearchRevoperationThis test function properly executes the test cases for the
SearchRevoperation, validating all expected aspects of its behavior.
602-632: Well-designed test cases forSearchRevwith pairsThese test cases properly verify the behavior of
SearchRevwith collections of pairs, ensuring that the operation works correctly with more complex data types.
634-647: Robust test function forSearchRevwith pairsThis test function properly executes the test cases for the
SearchRevoperation with collections of pairs, validating the expected behavior in such scenarios.
649-679: Comprehensive test cases forReduceRevoperationThe test cases cover the important scenarios for
ReduceRev, including empty collections (with error handling) and various non-empty collections.
681-694: Robust test function forReduceRevoperationThis test function properly executes the test cases for the
ReduceRevoperation, validating both the result value and error handling.definitions.go (3)
114-130: Good job moving order-dependent methods to the Linear interface.The migration of order-dependent methods like
EachRev,EachRevUntil,FindLast,FoldRev,ReduceRev, andSearchRevfromBasetoLinearinterface is excellent. This restructuring ensures that only collections guaranteeing element order provide these methods, which is architecturally sound.The documentation comments on methods like
FindLastare also clear and helpful.
138-139: Consider implementing the TODO methods or providing a timeline.There are TODO comments for two search methods:
SearchLastPosandSearchPosin theIndexedinterface. Since these appear to be related to the refactoring of order-dependent methods, it would be helpful to either implement them or document when they'll be addressed.Would you like to:
- Implement these methods in this PR?
- Create a separate issue to track them?
- Add more context about your plans for these methods?
129-130: Consider implementing the ValuesRev TODO method.Similar to the other TODO comment, there's a
ValuesRev()method marked with TODO. Since you've moved other reverse operations to theLinearinterface, implementing this method would complete the set of reverse operations.Would you like to implement this method now or create a tracking issue for it?
| type Base[V any] interface { | ||
|
|
||
| // Contains returns true if the collection contains an element that matches the predicate. | ||
| Contains(predicate Predicate[V]) bool | ||
|
|
||
| // Count returns the number of elements that match the predicate. | ||
| Count(predicate Predicate[V]) int | ||
|
|
||
| // Each iterates over the collection. | ||
| // This method is implicitly linear on all collections that also implement the Linear interface. | ||
| // There is no guarantee that the order of elements will be preserved if the collection does not implement | ||
| // the Linear interface. | ||
| // See also: EachUntil | ||
| Each(visit Visitor[V]) | ||
| EachRev(visit Visitor[V]) | ||
| EachRevUntil(valid Predicate[V]) | ||
|
|
||
| // EachUntil iterates over the collection until the predicate returns false. | ||
| // This method is implicitly linear on all collections that also implement the Linear interface. | ||
| // There is no guarantee that the order of elements will be preserved if the collection does not implement | ||
| // the Linear interface. | ||
| // See also: Each | ||
| EachUntil(valid Predicate[V]) | ||
|
|
||
| // Find returns the first element that matches the predicate, or the default value if no element matches. | ||
| // This method is implicitly linear on all collections that also implement the Linear interface. | ||
| // There is no guarantee that the order of elements will be preserved if the collection does not implement | ||
| // the Linear interface. | ||
| // See: Search | ||
| Find(predicate Predicate[V], defaultValue V) V | ||
| // FindLast returns the last element that matches the predicate, or the default value if no element matches. | ||
| // See: SearchRev | ||
| FindLast(predicate Predicate[V], defaultValue V) V | ||
|
|
||
| // Fold helps to fold a collection into a single value. | ||
| // Eg: Fold(func(acc int, i int, current int) int { return acc + current }, 0) // sum of all elements | ||
| // This method is implicitly linear on all collections that also implement the Linear interface. | ||
| // There is no guarantee that the order of elements will be preserved if the collection does not implement | ||
| // the Linear interface. | ||
| Fold(reducer Reducer[V], initial V) (result V) | ||
| FoldRev(reducer Reducer[V], initial V) (result V) | ||
|
|
||
| // IsEmpty returns true if the collection is empty. | ||
| IsEmpty() bool | ||
|
|
||
| // Len returns the number of elements in the collection. | ||
| Len() int | ||
|
|
||
| // Search returns the first element that matches the predicate, or the default value if no element matches. | ||
| Search(predicate Predicate[V]) (val V, found bool) | ||
| // SearchLastPos(predicate Predicate[V]) (val V, found bool) // TODO | ||
| // SearchPos(predicate Predicate[V]) (val V, found bool) // TODO | ||
| SearchRev(predicate Predicate[V]) (val V, found bool) | ||
|
|
||
| // Reduce helps to reduce a collection into a single value. | ||
| Reduce(reducer Reducer[V]) (result V, err error) | ||
| ReduceRev(reducer Reducer[V]) (result V, err error) | ||
|
|
||
| // ToSlice returns a slice of all elements in the collection. | ||
| // This method is implicitly linear on all collections that also implement the Linear interface. | ||
| // There is no guarantee that the order of elements will be preserved if the collection does not implement | ||
| // the Linear interface. | ||
| ToSlice() []V | ||
|
|
||
| // Values returns an iterator over all elements in the collection. | ||
| // This method is implicitly linear on all collections that also implement the Linear interface. | ||
| // There is no guarantee that the order of elements will be preserved if the collection does not implement | ||
| // the Linear interface. | ||
| Values() iter.Seq[V] | ||
| // ValuesRev() iter.Seq[V] // TODO | ||
| } |
There was a problem hiding this comment.
Interface refactoring looks good, but fix formatting issue on line 48.
The restructuring of the Base interface by adding methods like Contains, Count, Each, etc., and providing clear documentation for methods that are implicitly linear is a good design decision. This makes the interface more comprehensive for all collection types while clarifying behavior expectations.
However, there's a formatting issue at line 48 flagged by the pipeline: an empty line that doesn't meet Go formatting standards.
Run the following to fix the formatting:
-type Base[V any] interface {
-
+type Base[V any] interface {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type Base[V any] interface { | |
| // Contains returns true if the collection contains an element that matches the predicate. | |
| Contains(predicate Predicate[V]) bool | |
| // Count returns the number of elements that match the predicate. | |
| Count(predicate Predicate[V]) int | |
| // Each iterates over the collection. | |
| // This method is implicitly linear on all collections that also implement the Linear interface. | |
| // There is no guarantee that the order of elements will be preserved if the collection does not implement | |
| // the Linear interface. | |
| // See also: EachUntil | |
| Each(visit Visitor[V]) | |
| EachRev(visit Visitor[V]) | |
| EachRevUntil(valid Predicate[V]) | |
| // EachUntil iterates over the collection until the predicate returns false. | |
| // This method is implicitly linear on all collections that also implement the Linear interface. | |
| // There is no guarantee that the order of elements will be preserved if the collection does not implement | |
| // the Linear interface. | |
| // See also: Each | |
| EachUntil(valid Predicate[V]) | |
| // Find returns the first element that matches the predicate, or the default value if no element matches. | |
| // This method is implicitly linear on all collections that also implement the Linear interface. | |
| // There is no guarantee that the order of elements will be preserved if the collection does not implement | |
| // the Linear interface. | |
| // See: Search | |
| Find(predicate Predicate[V], defaultValue V) V | |
| // FindLast returns the last element that matches the predicate, or the default value if no element matches. | |
| // See: SearchRev | |
| FindLast(predicate Predicate[V], defaultValue V) V | |
| // Fold helps to fold a collection into a single value. | |
| // Eg: Fold(func(acc int, i int, current int) int { return acc + current }, 0) // sum of all elements | |
| // This method is implicitly linear on all collections that also implement the Linear interface. | |
| // There is no guarantee that the order of elements will be preserved if the collection does not implement | |
| // the Linear interface. | |
| Fold(reducer Reducer[V], initial V) (result V) | |
| FoldRev(reducer Reducer[V], initial V) (result V) | |
| // IsEmpty returns true if the collection is empty. | |
| IsEmpty() bool | |
| // Len returns the number of elements in the collection. | |
| Len() int | |
| // Search returns the first element that matches the predicate, or the default value if no element matches. | |
| Search(predicate Predicate[V]) (val V, found bool) | |
| // SearchLastPos(predicate Predicate[V]) (val V, found bool) // TODO | |
| // SearchPos(predicate Predicate[V]) (val V, found bool) // TODO | |
| SearchRev(predicate Predicate[V]) (val V, found bool) | |
| // Reduce helps to reduce a collection into a single value. | |
| Reduce(reducer Reducer[V]) (result V, err error) | |
| ReduceRev(reducer Reducer[V]) (result V, err error) | |
| // ToSlice returns a slice of all elements in the collection. | |
| // This method is implicitly linear on all collections that also implement the Linear interface. | |
| // There is no guarantee that the order of elements will be preserved if the collection does not implement | |
| // the Linear interface. | |
| ToSlice() []V | |
| // Values returns an iterator over all elements in the collection. | |
| // This method is implicitly linear on all collections that also implement the Linear interface. | |
| // There is no guarantee that the order of elements will be preserved if the collection does not implement | |
| // the Linear interface. | |
| Values() iter.Seq[V] | |
| // ValuesRev() iter.Seq[V] // TODO | |
| } | |
| type Base[V any] interface { | |
| // Contains returns true if the collection contains an element that matches the predicate. | |
| Contains(predicate Predicate[V]) bool | |
| // Count returns the number of elements that match the predicate. | |
| Count(predicate Predicate[V]) int | |
| // Each iterates over the collection. | |
| // This method is implicitly linear on all collections that also implement the Linear interface. | |
| // There is no guarantee that the order of elements will be preserved if the collection does not implement | |
| // the Linear interface. | |
| // See also: EachUntil | |
| Each(visit Visitor[V]) | |
| // EachUntil iterates over the collection until the predicate returns false. | |
| // This method is implicitly linear on all collections that also implement the Linear interface. | |
| // There is no guarantee that the order of elements will be preserved if the collection does not implement | |
| // the Linear interface. | |
| // See also: Each | |
| EachUntil(valid Predicate[V]) | |
| // Find returns the first element that matches the predicate, or the default value if no element matches. | |
| // This method is implicitly linear on all collections that also implement the Linear interface. | |
| // There is no guarantee that the order of elements will be preserved if the collection does not implement | |
| // the Linear interface. | |
| // See: Search | |
| Find(predicate Predicate[V], defaultValue V) V | |
| // Fold helps to fold a collection into a single value. | |
| // Eg: Fold(func(acc int, i int, current int) int { return acc + current }, 0) // sum of all elements | |
| // This method is implicitly linear on all collections that also implement the Linear interface. | |
| // There is no guarantee that the order of elements will be preserved if the collection does not implement | |
| // the Linear interface. | |
| Fold(reducer Reducer[V], initial V) (result V) | |
| // IsEmpty returns true if the collection is empty. | |
| IsEmpty() bool | |
| // Len returns the number of elements in the collection. | |
| Len() int | |
| // Search returns the first element that matches the predicate, or the default value if no element matches. | |
| Search(predicate Predicate[V]) (val V, found bool) | |
| // Reduce helps to reduce a collection into a single value. | |
| Reduce(reducer Reducer[V]) (result V, err error) | |
| // ToSlice returns a slice of all elements in the collection. | |
| // This method is implicitly linear on all collections that also implement the Linear interface. | |
| // There is no guarantee that the order of elements will be preserved if the collection does not implement | |
| // the Linear interface. | |
| ToSlice() []V | |
| // Values returns an iterator over all elements in the collection. | |
| // This method is implicitly linear on all collections that also implement the Linear interface. | |
| // There is no guarantee that the order of elements will be preserved if the collection does not implement | |
| // the Linear interface. | |
| Values() iter.Seq[V] | |
| } |
🧰 Tools
🪛 GitHub Check: test-and-lint
[failure] 48-48:
File is not properly formatted (gofumpt)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
definitions.go (1)
137-139: Consider prioritizing the TODO methods.There are TODO comments for
SearchLastPosandSearchPosmethods in theIndexedinterface. Since you're already restructuring interfaces to better represent order-dependent operations, it might be worth implementing these methods now to complete the refactoring.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
definitions.go(3 hunks)
🔇 Additional comments (3)
definitions.go (3)
47-104: Interface restructuring improves design clarity.The restructuring of the
Baseinterface with comprehensive documentation is excellent. By adding detailed explanations for methods likeEach,EachUntil,Find, etc., you've clarified which operations are implicitly linear and which ones don't guarantee element order preservation. This makes the interface boundaries much clearer and will help users understand the behavior they can expect.The addition of methods like
Contains,Count,IsEmpty,Len, etc. to the base interface ensures that fundamental operations are available on all collections while keeping order-dependent operations separate.
115-129: Good migration of order-dependent methods to Linear interface.Moving the reverse operation methods (
EachRev,EachRevUntil,FindLast,FoldRev,ReduceRev,SearchRev) to theLinearinterface aligns perfectly with the PR objective. These operations only make sense for collections that maintain element order, so this is the right place for them.I notice there are TODO comments for potential future methods (
LinearValues()andValuesRev()). Consider creating issues to track these planned enhancements if they haven't been created yet.
48-48: Fixed formatting issue from previous review.The formatting issue on line 48 that was flagged in the previous review has been properly addressed.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests