#13 - RemoveMatching methods return the number of removed items#31
#13 - RemoveMatching methods return the number of removed items#31
Conversation
WalkthroughThe changes update several components to improve the reporting of the number of items removed by the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Container as RemoveMatching
Caller->>Container: Call RemoveMatching(predicate)
Container->>Container: Iterate through collection
alt Element matches predicate
Container->>Container: Increment removal count & remove element
else
Container->>Container: Retain element
end
Container->>Caller: Return (modified container, removal count)
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 (1)
🔇 Additional comments (1)
✨ 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
🔭 Outside diff range comments (1)
map_cases_test.go (1)
1537-1563:⚠️ Potential issueMissing validation of returned count in test function.
The test function doesn't capture or validate the count returned by
RemoveMatchingagainst the expectedwant5value in each test case.Apply this diff to fix the issue:
func testMapRemoveMatching(t *testing.T, builder baseMapCollIntBuilder) { cases := getMapRemoveMatchingCases(builder) for _, tt := range cases { t.Run(tt.name, func(t *testing.T) { - tt.coll.RemoveMatching(tt.args.predicate) + count := tt.coll.RemoveMatching(tt.args.predicate) actualSlice := builder.extractUnderlyingSlice(tt.coll) actualMap := builder.extractUnderlyingMap(tt.coll) actualKP := builder.extractUnderlyingKp(tt.coll) actualVC := builder.extractUnderlyingValsCount(tt.coll) if !reflect.DeepEqual(actualSlice, tt.want1) { t.Errorf("RemoveMatching() did not remove correctly from slice") } if !reflect.DeepEqual(actualMap, i2iToPairs(tt.want2)) { t.Errorf("RemoveMatching() did not remove correctly from map") } if !reflect.DeepEqual(actualKP, tt.want3) { t.Errorf("RemoveMatching() did not remove correctly from kp") } if actualVC != nil { if !reflect.DeepEqual(actualVC, tt.want4) { t.Errorf("RemoveMatching() did not remove correctly from values counter") } } + if count != tt.want5 { + t.Errorf("RemoveMatching() returned wrong count: %v, but wanted = %v", count, tt.want5) + } }) } }
🧹 Nitpick comments (1)
mapcmp.go (1)
193-217: Implementation correctly tracks and returns removed item count.The changes properly implement the requirement to return the count of removed items. The
countvariable is incremented when an element matches the predicate and is correctly returned at the end of the method.However, I notice that the
RemoveValuesmethod on lines 219-223 callsRemoveMatchingbut doesn't capture the returned count. Consider updating this method to also return the count for consistency.-func (c *comfyCmpMap[K, V]) RemoveValues(v V) { - c.RemoveMatching(func(pair Pair[K, V]) bool { - return pair.Val() == v - }) +func (c *comfyCmpMap[K, V]) RemoveValues(v V) int { + return c.RemoveMatching(func(pair Pair[K, V]) bool { + return pair.Val() == v + }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
common_test.go(1 hunks)definitions.go(1 hunks)functions_internal.go(1 hunks)map.go(2 hunks)map_cases_test.go(8 hunks)mapcmp.go(2 hunks)mutable_cases_test.go(3 hunks)sequence.go(1 hunks)sequencecmp.go(1 hunks)
🔇 Additional comments (9)
common_test.go (1)
37-37: Adding field to support the RemoveMatching count return valueThe addition of the
want5field to thetestCasestruct provides a place to store the expected count of removed items for testing the updatedRemoveMatchingfunctionality. This aligns with the PR objective to return the number of removed items.definitions.go (1)
92-92: Enhancement to RemoveMatching interfaceGood change - updating the
RemoveMatchingmethod in theMutableinterface to return the count of removed elements provides valuable information to callers. This enhancement is consistent with the PR objective (#13) and improves the API usability.sequence.go (1)
96-99: Implementation of RemoveMatching with count returnThe updated implementation correctly leverages the
sliceRemoveMatchinghelper function to handle both updating the sequence and returning the count of removed elements. The implementation is clean and straightforward.sequencecmp.go (1)
143-157: Implementation of RemoveMatching with count for CmpSequenceThe implementation correctly tracks and returns the count of removed elements while maintaining the value counter. This approach differs slightly from the regular sequence implementation but is appropriate given the need to update the value counter.
functions_internal.go (1)
61-77: LGTM! The function now returns the count of removed items.The changes appropriately modify the function signature to return both the filtered slice and a count of removed items. The implementation correctly increments the counter each time an element is removed and returns this count alongside the filtered slice.
mutable_cases_test.go (2)
106-164: LGTM! Test cases properly updated with expected removal counts.The test cases have been correctly updated with a new
want3field that tracks the expected number of items removed by theRemoveMatchingoperation. The counts are accurate for each test case:
- 0 for empty collections or when no items match
- Proper counts for partial and full matches
167-185: LGTM! Test function now validates the returned count.The test function has been properly updated to capture the return value from
RemoveMatchingand verify it against the expected count. This completes the testing of the new functionality.map.go (1)
151-172: LGTM! The method now returns the count of removed items.The implementation has been updated to track and return the count of elements that match the predicate. The method correctly increments the counter when an element is removed and returns the final count.
map_cases_test.go (1)
1442-1533: LGTM! Test cases properly updated with expected removal counts.The test cases have been correctly updated with a new
want5field that tracks the expected number of items removed by theRemoveMatchingoperation. The added test case for finding and removing items with odd values is a good addition to the test coverage.
1 participant
Summary by CodeRabbit
New Features
Tests