#13 - RemoveValues accepts multiple items and returns number of removed items#32
#13 - RemoveValues accepts multiple items and returns number of removed items#32
Conversation
WalkthroughThe update refines the documentation language and enhances the removal functionalities across multiple collection types. The changes update method signatures to accept variadic parameters and return the count of removed items. Both map-based and sequence-based implementations of the removal methods have been adjusted, with corresponding test cases expanded to cover these scenarios. Additionally, new utility methods for managing value counts have been introduced along with their tests. No alterations were made to the exported or public entities outside of updated comments and method signatures. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Caller
participant Collection as Collection (Map/Seq)
Client->>Collection: RemoveValues(v1, v2, ... )
Collection->>Collection: Build removal counter (toRemove)
alt toRemove is empty
Collection-->>Client: Return count = 0
else
Collection->>Collection: Iterate over items and apply removal predicate
Collection->>Collection: Decrement counter and accumulate removal count
Collection-->>Client: Return total removal count
end
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)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (12)
✨ 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: 0
🧹 Nitpick comments (2)
sequencecmp.go (1)
163-168: Consider optimizing counter initializationThe current implementation always creates a new counter and then checks each value individually. You could optimize this by only initializing the counter when at least one value to remove exists.
- toRemove := newValuesCounter[V]() - for _, v := range v { - if c.vc.Count(v) > 0 { - toRemove.counter[v] = c.vc.Count(v) - } - } + var toRemove *valuesCounter[V] + for _, v := range v { + if c.vc.Count(v) > 0 { + if toRemove == nil { + toRemove = newValuesCounter[V]() + } + toRemove.counter[v] = c.vc.Count(v) + } + } + + if toRemove == nil { + return 0 + }cmpmutable_cases_test.go (1)
133-138: Consider simplifying the test logic for RemoveValuesThe conditional approach to call RemoveValues with either a single value or multiple values could be simplified. Since RemoveValues now accepts variadic parameters, you can simply handle all cases uniformly.
- count := -1 - if tt.args.values != nil { - count = tt.coll.RemoveValues(tt.args.values...) - } else { - count = tt.coll.RemoveValues(tt.args.value) - } + var count int + if tt.args.values != nil { + count = tt.coll.RemoveValues(tt.args.values...) + } else { + count = tt.coll.RemoveValues(tt.args.value) + }Or even better, consider updating the
testArgsstruct to use a standardized approach for all cases:// In the args struct definition (not shown in this file) type testArgs[C any, V any] struct { - value V - values []V + values []V // Use this for all cases } // Then update all test cases to use values instead of value // And simplify the test function: count := tt.coll.RemoveValues(tt.args.values...)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.adoc(1 hunks)cmp.go(2 hunks)cmpmutable_cases_test.go(3 hunks)definitions.go(2 hunks)mapcmp.go(1 hunks)sequencecmp.go(1 hunks)
🔇 Additional comments (10)
cmp.go (2)
3-5: Import formatting looks good.The multi-line import format makes the code more maintainable as additional imports are added in the future.
37-39: Great addition of the IsEmpty method.This helper method provides a clean way to check if the counter contains any entries, improving the API's usability. The implementation correctly checks if the length of the counter map is zero.
README.adoc (1)
21-21: Improved goal description.Changing from "day-to-day" to "common" collections operations provides a clearer description of the package's intent.
mapcmp.go (1)
219-234: Well-implemented enhancement to RemoveValues.The method now accepts variadic parameters to remove multiple values in a single call and returns the count of removed items. The implementation is efficient:
- Creates a counter for values to be removed
- Only targets values that exist in the collection
- Uses RemoveMatching with a predicate for actual removal
- Properly tracks and decrements the removal counter
This change significantly improves the API's flexibility and provides valuable feedback to callers.
definitions.go (2)
92-93: Helpful clarification on return value.Adding this comment clarifies the behavior of RemoveMatching by documenting that it returns the number of removed items.
139-140: Interface properly updated for RemoveValues.The CmpMutable interface has been correctly updated to:
- Accept variadic parameters (...V)
- Return the count of removed items
- Include a clarifying comment
This change aligns with the implementation in mapcmp.go and provides a more flexible API.
sequencecmp.go (2)
159-188: Good implementation of variadic RemoveValues with count returnThe refactored
RemoveValuesmethod now supports removing multiple values at once and returns the count of removed items. The implementation is efficient, using a counter approach to track removals.A few observations:
- The use of
toRemove.IsEmpty()as an early exit optimization is good.- The counter-based approach properly handles duplicates in both the collection and the input values.
170-172:Details
✅ Verification successful
Verify IsEmpty method exists on valuesCounter
The code references
toRemove.IsEmpty(), but this method is not visible in the provided code. Ensure this method exists on thevaluesCounterstruct.
🏁 Script executed:
#!/bin/bash # Check if IsEmpty() method exists on valuesCounter struct grep -A 5 -B 5 "func.*valuesCounter.*IsEmpty" --include="*.go" -r .Length of output: 248
IsEmpty method verified: No action needed
The execution confirmed that the
IsEmpty()method exists on thevaluesCounterstruct (defined incmp.go), so no changes are necessary insequencecmp.go.cmpmutable_cases_test.go (2)
86-125: Good comprehensive test coverage for variadic RemoveValuesThe new test cases thoroughly cover various scenarios for multi-value removal, including:
- Single value removal (using variadic syntax)
- Multiple value removal with different quantities
- Edge cases like empty value slices and non-existent values
This comprehensive test coverage ensures the new functionality works correctly across different scenarios.
147-149: Good validation of return valueThe added verification of the returned count is essential for testing the new functionality. It ensures that the method not only modifies the collection correctly but also reports the number of removed items accurately.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
mapcmp.go (1)
219-237: Excellent implementation of variadic RemoveValues function!The method has been updated to accept multiple values and return the count of removed items. The implementation uses an efficient approach by:
- Tracking values to be removed with a counter
- Early returning if nothing to remove
- Leveraging the existing RemoveMatching method
One minor suggestion: consider using encapsulation methods instead of direct access to the counter's internal data structure on line 223:
- toRemove.counter[v] = c.vc.Count(v) + // Assuming there's an appropriate method like: + toRemove.Set(v, c.vc.Count(v))
1 participant
Summary by CodeRabbit
Documentation
New Features