Conversation
…ed valuesCounter
Codecov ReportAttention: Patch coverage is
🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes update the module configuration and documentation, refactor core collection APIs and internal type signatures, and enhance extensive test suites. New generic types, interfaces, and methods such as a values counter (in Changes
Sequence Diagram(s)Below is a high-level sequence diagram outlining the interactions when processing collection operations with value counting and removal: sequenceDiagram
participant Client
participant Collection
participant ValuesCounter
Client->>Collection: Append(item)
Collection->>ValuesCounter: Increment(item)
ValuesCounter-->>Collection: Updated count
Client->>Collection: RemoveAt(index)
Collection->>ValuesCounter: Decrement(removed_item)
ValuesCounter-->>Collection: Updated count
Client->>Collection: GetItem(index)
Collection-->>Client: Returns item
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) 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
sequence_test.go (1)
287-291:⚠️ Potential issueLogic error in deep copy validation loop.
for v := range c2.(*comfySeq[int]).s { if v == 999 { t.Error("copy() did not create a deep copy") } }Here,
vis the index, not the slice element. This mistakenly checks for999in the index. Use something likefor i, val := range c2.(*comfySeq[int]).s { ... }or comparevaldirectly to ensure correctness.
🧹 Nitpick comments (26)
map_pairref_cases_test.go (2)
108-152: Added tests to verify copy semantics for mapsThese tests ensure that copying a map creates a deep copy where modifying values in the copy doesn't affect the original. This is important for maintaining collection immutability when expected.
One suggestion: the test name "Values() on three-item collection, modify values" at line 110 doesn't clearly reflect that this is testing copy semantics. Consider renaming it to something like "Copy() creates independent pairs that don't affect the original".
141-144: Verify type assertion safety in copy operationThe code performs a type assertion without checking if it's successful:
copiedMap := tt.coll.copy().(mapInternal[int, int])Consider adding a type assertion check to handle potential runtime errors gracefully:
-copiedMap := tt.coll.copy().(mapInternal[int, int]) +copied, ok := tt.coll.copy().(mapInternal[int, int]) +if !ok { + t.Fatalf("Failed to cast copied collection to mapInternal[int, int]") +} +copiedMap := copieddefinitions_internal.go (2)
1-4: Establish consistent package documentation.The package-level comment is concise, but consider including a brief overview of the collection interfaces and their usage to help users quickly grasp the library’s purpose.
82-86: Consider clarity around comparison constraints in map-based interfaces.These interfaces (
cmpMapBaseInternal) couple key comparison (viacomparable) withcmp.Orderedfor values. Verify this combined requirement is intentional. If value ordering is not always needed, splitting them might improve flexibility.indexedmutable_cases_test.go (4)
37-39: Consistency in verifying removed elements.Again, verifying the “removed” element in the test is a robust check. Keep consistent naming for test variables for clarity.
45-47: Check for ordering in the final array.Verifying the final slice after removal ensures correctness. Consider also testing corner cases such as repeated removals at the same index.
74-81: Validate duplicates handling.The test “RemoveAt() on six-item with duplicates” is indispensable for correctness. Looks good; further coverage might include removing from the start/end in duplicates scenario, if not already tested.
150-154: Confirm test coverage for Sort.You updated the expected sorted slice to
[]int(nil)in certain cases (empty). This is consistent. Ensure any usage across the code respectsnilvs. empty slice differences.linearmutable_cases_test.go (1)
100-110: Consolidate repeated logic.
testAppendMany()and related test blocks reuse a pattern of extracting slice and value-count. If the pattern grows, consider a small helper to reduce duplication.sequencecmp.go (2)
38-41: Increment counters after each append.The loop approach is straightforward. If performance becomes critical for large slices, consider a batch increment approach in the future.
174-174: Increment after insert.Slicing logic plus increment for the inserted item is consistent. Watch out for future concurrency needs, though not a concern here.
Also applies to: 178-179
sequencecmp_test.go (1)
1-1: Nolint comment noted.Ensure the linter exceptions are intentional or remove if unneeded.
cmpmutable_cases_test.go (1)
229-232: Mismatched error messages for SortDesc.
Although functionally correct, the error messages reference “SortAsc()” instead of “SortDesc()”. Consider updating for clarity.- t.Errorf("SortAsc() resulted in: %v, but wanted %v", actualSlice, tt.want1) + t.Errorf("SortDesc() resulted in: %v, but wanted %v", actualSlice, tt.want1) - t.Errorf("SortAsc() did not sort correctly from values counter") + t.Errorf("SortDesc() did not sort correctly from values counter")map.go (1)
16-17: Outdated doc comment regarding NewMapFrom.
Lines 16-17 claim that noNewMapFromconstructor exists, but it's actually defined at line 25. Consider removing or updating this comment for correctness.- // Note that there is no NewMapFrom constructor, because it would create collection in random order. + // Creates a new Map in the order of the provided slice of pairs.functions_internal.go (4)
35-40: Add dedicated tests forcomfyContainsValue.
These lines introduce a new helper without coverage, as indicated by static analysis. Adding a direct test ensures no regressions in matching logic.Would you like help generating a test function snippet for this utility?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 35-38: functions_internal.go#L35-L38
Added lines #L35 - L38 were not covered by tests
41-44:comfyContainsKValso lacks patch coverage.
Consider adding a targeted unit test that invokescomfyContainsKVwith various key-value predicates.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 41-41: functions_internal.go#L41
Added line #L41 was not covered by tests
[warning] 43-44: functions_internal.go#L43-L44
Added lines #L43 - L44 were not covered by tests
160-160: Missing coverage for else-if path.
If the new value is not less than the accumulator, confirm you have a test that exercises this path incomfyMinOfPairs.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 160-160: functions_internal.go#L160
Added line #L160 was not covered by tests
180-184: Lack of test coverage incomfyFoldSliceRev.
These lines iterate items in reverse, but might not be invoked in your tests.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 180-180: functions_internal.go#L180
Added line #L180 was not covered by tests
[warning] 182-184: functions_internal.go#L182-L184
Added lines #L182 - L184 were not covered by testsdefinitions.go (4)
37-38: Name mismatch in comment vs. type.
The comment states "KKVistor" whereas the type name isKVVistor. Consider updating one or the other to maintain naming consistency.
72-73: TODO comment for reverse values iteration.
A placeholder exists forValuesRev(). If reverse iteration is critical, consider implementing to avoid code drift.
87-87: Commented-out TODO.
// LinearValues()indicates an intended feature. Make sure to address or remove this TODO to keep the codebase clean.
145-145: Multi-value removal is hinted as a TODO.
RemoveValues(v V)references replacing with...V. This can be valuable for batch operations. Could be worth implementing to align with flexible usage.map_test.go (2)
64-71: Add nil check for slice before attempting extractionThe
extractRawValuesmethod doesn't check if the slice is nil before attempting to iterate through it. This could cause a panic ifextractUnderlyingSlicereturns a nil slice.Recommended change:
func (lcb *comfyMapIntBuilder[C]) extractRawValues(c C) any { s := lcb.extractUnderlyingSlice(c).([]Pair[int, int]) + if s == nil { + return []int(nil) + } flat := make([]int, 0, len(s)) for _, pair := range s { flat = append(flat, pair.Val()) } return flat }
85-87: Document why this method always returns nilThe
extractUnderlyingValsCountmethod always returns nil, which suggests that thecomfyMapimplementation doesn't maintain a value counter (unlikecomfyCmpMap). Consider adding a comment to explain why this is intentional.func (lcb *comfyMapIntBuilder[C]) extractUnderlyingValsCount(_ C) any { + // comfyMap doesn't implement a values counter return nil }mapcmp_test.go (2)
88-94: Consider a more graceful error handling approachThe current implementation panics if the values counter is nil. While panicking is acceptable in tests, consider a more graceful approach like returning nil or a default value, especially for a utility method.
func (lcb *comfyCmpMapIntBuilder[C]) extractUnderlyingValsCount(c C) any { vc := (any(c)).(*comfyCmpMap[int, int]).vc.counter if vc == nil { - panic("Could not extract Values Counter from comfyCmpMap") + // Log an error instead of panicking + return map[int]int{} } return vc }
393-406: Initialize all required fields in the test objectThe
comfyCmpMapinstance in this test is missing thesandkpfields that are initialized inmakeand other tests. While this might not affect the specific test cases here, it's better to fully initialize objects for consistency.c1 := &comfyCmpMap[int, int]{ + s: []Pair[int, int]{ + NewPair(1, 111), + NewPair(2, 222), + NewPair(3, 333), + }, m: map[int]Pair[int, int]{ }, + kp: map[int]int{ + 1: 0, + 2: 1, + 3: 2, + }, vc: &valuesCounter[int]{ counter: map[int]int{ }, }, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
.idea/collections.iml(1 hunks)README.adoc(1 hunks)base_cases_test.go(9 hunks)cmp.go(1 hunks)cmp_cases_test.go(10 hunks)cmpmutable_cases_test.go(4 hunks)common_test.go(1 hunks)definitions.go(9 hunks)definitions_internal.go(1 hunks)functions.go(1 hunks)functions_internal.go(5 hunks)indexed_cases_test.go(1 hunks)indexedmutable_cases_test.go(4 hunks)linear_cases_test.go(1 hunks)linearmutable_cases_test.go(14 hunks)list_cases_test.go(3 hunks)map.go(2 hunks)map_pairref_cases_test.go(1 hunks)map_test.go(1 hunks)mapcmp.go(1 hunks)mapcmp_test.go(1 hunks)mutable_cases_test.go(6 hunks)sequence.go(5 hunks)sequence_test.go(4 hunks)sequencecmp.go(8 hunks)sequencecmp_test.go(6 hunks)set.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
functions.go
[warning] 6-6: functions.go#L6
Added line #L6 was not covered by tests
functions_internal.go
[warning] 35-38: functions_internal.go#L35-L38
Added lines #L35 - L38 were not covered by tests
[warning] 41-41: functions_internal.go#L41
Added line #L41 was not covered by tests
[warning] 43-44: functions_internal.go#L43-L44
Added lines #L43 - L44 were not covered by tests
[warning] 146-147: functions_internal.go#L146-L147
Added lines #L146 - L147 were not covered by tests
[warning] 160-160: functions_internal.go#L160
Added line #L160 was not covered by tests
[warning] 180-180: functions_internal.go#L180
Added line #L180 was not covered by tests
[warning] 182-184: functions_internal.go#L182-L184
Added lines #L182 - L184 were not covered by tests
[warning] 208-208: functions_internal.go#L208
Added line #L208 was not covered by tests
[warning] 210-210: functions_internal.go#L210
Added line #L210 was not covered by tests
[warning] 215-215: functions_internal.go#L215
Added line #L215 was not covered by tests
[warning] 217-217: functions_internal.go#L217
Added line #L217 was not covered by tests
[warning] 220-220: functions_internal.go#L220
Added line #L220 was not covered by tests
🔇 Additional comments (295)
.idea/collections.iml (1)
5-7: IDE configuration update looks good.The change to exclude the
vardirectory from the module's content is a standard IDE configuration update that won't affect runtime behavior. This is useful for excluding build artifacts or temporary files from IDE indexing.set.go (1)
9-9: Interface simplification is a good improvement.Simplifying the
CmpMaptype parameter fromCmpMap[*comfyPair[K, V], K, V]toCmpMap[K, V]removes the dependency on the implementation detail (comfyPair) and makes the interface cleaner. This change aligns with Go's philosophy of simple, focused interfaces.README.adoc (3)
1-3: Clear title and description improvement.The updated title "What is Comfy Gopher?" and enhanced description provide better context about the project's purpose. This makes it immediately clear to new users what they're looking at.
9-18: Well-defined goals provide clear direction.The explicit listing of goals for the Collections package is excellent. It helps users understand the specific problems the library aims to solve, such as providing abstractions for collections, reducing repetition, and addressing the missing ordered map data structure.
19-29: Important limitations clearly communicated.Explicitly stating the non-goals (thread-safety and exceptional efficiency) sets appropriate expectations for users. This honesty about limitations is a best practice in library documentation and helps prevent misuse.
linear_cases_test.go (1)
8-10: Type alias refactoring looks good!The change from
Linear[int]tolinearInternal[int]in these type aliases aligns with good testing practices by focusing on testing implementation details directly. This refactoring is consistent with similar changes in other test files and maintains the same test behavior while providing more precise testing of internal structures.indexed_cases_test.go (1)
8-10: Type alias refactoring looks good!The change from
Indexed[int]toindexedInternal[int]in these type aliases follows the same pattern as in other test files. This refactoring improves consistency in the testing framework while maintaining the same test functionality.sequence.go (7)
27-29: Good optimization for empty input slicesAdding this early return for empty slice inputs avoids unnecessary operations and improves performance.
61-61: Efficient memory usage with nil slicesUsing
[]V(nil)instead of an empty slice with capacity provides better memory efficiency as it resets the underlying array.
128-128: Method implementation refactoring looks goodChanging to
comfyFoldSlicesuggests a more specialized implementation for slices, which should improve code organization and maintainability.
166-168: Good optimization for empty input slicesSimilar to the change in
Append, this early return for empty slice inputs avoids unnecessary operations and improves performance.
173-173: Method implementation refactoring looks goodSimilar to the
Foldmethod change, usingcomfyReduceSliceprovides a more specialized implementation for slices.
176-182: API improvement with better return valuesThe updated signature improves API consistency by returning the removed element along with an error. The implementation is also cleaner by directly assigning the results of
sliceRemoveAt.
249-259: Properly implemented copy methodThe reinstated
copymethod correctly creates a deep copy of the sequence. The//nolint:unuseddirective is appropriate for methods that may be used in the future or required by interfaces but aren't currently called.cmp.go (4)
5-7: Good generic type constraint for counterUsing
cmp.Orderedas a constraint ensures that the counter works with any comparable and orderable type, which is appropriate for a map key.
9-13: Clean constructor initializationThe constructor properly initializes the
countermap, ensuring that the struct is ready to use immediately after creation.
15-20: Proper handling of non-existent keysThe
Countmethod correctly checks for key existence before accessing the map value, returning 0 for non-existent keys.
22-28: Good increment implementation with key existence checkThe
Incrementmethod properly handles both existing and non-existing keys, initializing new entries correctly.list_cases_test.go (4)
9-11: Updated type signatures to use internal implementation instead of exported typesThe type definitions have been changed to use
listInternalinstead ofList, reflecting a shift to testing the internal implementation rather than the exported API surface. This approach allows for more thorough testing of implementation details.
20-20: Added value counter verification for test casesThe test cases now include a
want2field containing a map that tracks the expected value counts after insertion. This provides an additional validation mechanism to ensure the internal counting mechanism works correctly.Also applies to: 27-27, 34-34
64-77: Added test cases for collections with duplicatesThese new test cases are valuable additions that verify insertion behavior with collections containing duplicate values, testing both appending at the end and insertion in the middle of the collection.
125-134: Enhanced verification logic for test casesThe verification logic now properly extracts the underlying slice and value counts using the builder's extraction methods. This matches the updated test case structure and provides more thorough verification of the internal state after operations.
common_test.go (4)
3-19: Enhanced testArgs structure with additional fieldsThe
testArgsstructure has been expanded with new fields to support more comprehensive test cases:
keyandkeysfor testing map-like operationsdefaultRawValuefor testing with untyped valuesintPredicatefor testing integer-specific predicatesThese additions enable more versatile test scenarios, particularly for the new map collections.
21-27: Added helper method for type conversionThe new
valuesAsAnySlicemethod provides a convenient way to convert typed values toanyslices, which is useful for testing generic collection builders that accept interface{} values.
29-45: Enhanced testCase structure with additional fieldsThe
testCasestructure has been expanded with:
collBuilderfor dynamic collection creationwant4for additional validationmetaInt1for numeric metadatamodifyfunction for testing mutation operationsThese additions support more complex test scenarios, particularly for reference type testing.
47-61: Improved testCollectionBuilder interface with extraction methodsThe interface has been simplified by removing the type parameter
Vand enhanced with:
ThreeRev()for reversed collection testingFromValues()for creating collections from arbitrary values- Various extraction methods for accessing internal collection state
These changes enable more thorough testing of implementation details while maintaining a clean public API.
mutable_cases_test.go (6)
8-10: Updated type signatures to use internal implementationSimilar to the changes in
list_cases_test.go, the type definitions have been updated to usemutableInternalinstead ofMutable, allowing for better testing of internal implementation details.
18-19: Updated empty collection representation and added value counter verificationEmpty collections are now represented as
[]int(nil)instead of[]int{}, and value counter maps have been added for verification. This ensures consistency in how empty collections are represented and adds an additional layer of validation.Also applies to: 26-26
35-41: Added test case for collections with duplicatesThis new test case verifies the Apply operation on collections with duplicate values, ensuring that the mapper function correctly transforms each value and the value counter properly tracks the result.
51-59: Enhanced verification logic for Apply testsThe verification logic now extracts and validates both the underlying slice and the value counts using the builder's extraction methods, providing more thorough validation of the internal state after operations.
93-101: Enhanced verification logic for Clear testsSimilar to the Apply tests, the Clear test verification has been updated to validate both the underlying slice and value counts, ensuring that Clear properly resets all internal data structures.
173-181: Enhanced verification logic for RemoveMatching testsThe RemoveMatching test verification has been updated to validate both the underlying slice and value counts, ensuring that elements are properly removed from all internal data structures.
map_pairref_cases_test.go (2)
8-62: Added comprehensive test cases for map append operations with reference typesThese tests verify that appending key-value pairs to maps works correctly, especially when the pairs contain mutable reference values. The tests include:
- Verification of both slice and map representations
- Testing reference modification after append
- Testing behavior with both empty and non-empty collections
This is a valuable addition that ensures reference semantics are handled correctly in map collections.
64-106: Added tests for map value modification through iterationThese tests verify that modifying values obtained through iteration properly updates the underlying collection. This is critical for ensuring that the
Values()method returns references that are connected to the original collection, not detached copies.definitions_internal.go (3)
10-14: Interfaces appear well-structured.The
baseInternalinterface cleanly specifies acopymethod on top of theBase[V]operations. This design is consistent and shows good separation of concerns.
88-110:comfyPairstruct design looks clean.The
Key(),Val(),SetVal(), andcopy()methods are straightforward. Consider adding a doc comment forcopy()to clarify that it returns a new instance, distinct from the original.
66-74:Details
✅ Verification successful
Validate naming and usage for map-specific operations.
Methods like
prependAllorremoveManyare somewhat unusual for a map abstraction. Ensure these method names match your domain’s conventions and do not conflict with typical map usage patterns.Do you already have tests that confirm these operations behave as intended across edge cases (e.g., empty map, duplicate keys)? We can verify them by scanning the test files:
🏁 Script executed:
#!/bin/bash # Searching for unit tests referring to prependAll, removeMany, or set rg -A 5 -B 3 '(prependAll|removeMany|set)\('Length of output: 6540
Final Verification of Map Operation Naming
The investigation confirms that our domain-specific naming (e.g.,
prependAll,removeMany, andset) is consistently used and thoroughly tested. The unit tests in files such asmap.go,mapcmp_test.go, andmap_test.goverify their behavior across typical edge cases (empty inputs, duplicate keys, etc.), ensuring that the operations perform as intended and align with our project's conventions.
- Consistency Check: Naming conventions in
definitions_internal.gomatch those used throughout the codebase.- Test Coverage: Existing tests cover these methods, confirming correct behavior in edge cases.
- Recommendation: No modifications are needed. Just ensure that future changes remain consistent with these established patterns.
indexedmutable_cases_test.go (9)
9-11: Good approach to unify type definitions.Using
indexedMutableInternal[int]clarifies the test coverage for these internal structures without duplicating code.
19-21: Ensure zero-value usage is intentional.Using
want2 = 0in the test indicates the removed value is expected to be zero when out of bounds. Confirm that this behavior aligns with your error handling approach and default values.
28-31: Verifying new want fields.You’ve introduced
want2(the removed value) andwant3(value counts). This extra detail helps ensure correctness thoroughly—nice improvement for test clarity.
53-55: Comprehensive approach for want3.Tracking value counts after each removal helps catch potential index shift or repeated element issues. Good thoroughness.
61-63: Edge case coverage.Ensuring out-of-bounds indices check both scenarios (negative and too large) is vital. Great coverage.
70-72: Negative index test is consistent.The negative index path returning an error and zero removed value is consistent with the out-of-bounds scenario. Good defensive testing.
89-89: Removed value assignment.Assigning
removed, err := tt.coll.RemoveAt(...)clarifies the function’s new return signature. Good usage.
99-110: Solid verification of slice and counter.Ensuring
actualSliceandactualVCmatch expectations is thorough. Great test design for internal state checks.
121-121: No changes to review at this line.Skipping further commentary since there's no additional logic.
linearmutable_cases_test.go (9)
8-10: Refactor to uselinearMutableInternal[int].Similar to the indexed case, switching to
linearMutableInternal[int]ensures synergy with the newly introduced internal interfaces.
19-19: Explicitly track counts inwant2.Adding a
map[int]intis beneficial for verifying final state. This detail can catch duplication or missed increments in the library code.
26-27: Verify new item count.
map[int]int{111: 1, 1: 1}in the test ensures appended items are tracked. Useful for ensuring internal counters are correct.
33-34: Meaningful test coverage for multi-item append.Checking a mix of existing and new values enhances reliability.
44-46: Ensure big appends respect capacity.Appending multiple items to the underlying slice can trigger expansions. The test coverage plus value-count verification is robust.
Also applies to: 50-51
64-65: Edge-case coverage for appending none.Verifying appending empty slices to both empty and non-empty collections ensures no side effects. Good practice.
Also applies to: 71-72, 78-79, 84-86, 91-93
121-121: Slice and counts after single-item append from another collection.Checks consistency across collection boundaries. Nicely done.
Also applies to: 123-123
130-131: Appending collections and verifying final distribution.The approach ensures that appended elements are all accounted for in both slice order and value counts. Looks thorough.
Also applies to: 137-138, 143-145, 150-152
182-183: Prepend tests mirror Append’s thoroughness.Ensuring both forward and reverse insertion operations handle counts properly is well covered.
Also applies to: 189-190, 195-197
sequencecmp.go (17)
12-13: Initialization with empty slice and new counter.Good practice to keep the slice
nilinitially while still having a valid counter. Simplifies memory usage for empty sequences.
18-21: Constructing from slice.Creating a fresh sequence and appending the elements leverages shared logic and ensures counters are incremented properly.
25-26: Track sequence data withvc.Storing a
*valuesCounter[V]is consistent with the entire design. This automatically updates counts on operations.
31-31: Mapping function updates.Decrement the old value, apply the transform, then increment the new value—this ensures correct internal counts.
Also applies to: 33-34
45-48: Appending from another collection.Ensuring each item is appended and counted individually is consistent with your design. Looks good.
66-68: Re-initialize counters when clearing.Resetting the slice and reassigning a fresh counter thoroughly cleans up existing references. Safe approach.
72-72: No changes at line 72.Skipping further commentary since there's no alteration here.
90-91:CountValuesusage.Directly returning the counter’s count is significantly more efficient than scanning the slice. Good improvement.
140-141: LeveragecomfyFoldSlice.Delegating the fold logic fosters reuse. The approach is consistent with your pattern in
Fold,Reduce, and other aggregator methods.
143-145:HasValuedelegates toContainsValue.This small convenience method is clean and prevents repeated logic.
163-165: Short-circuit zero counts inIndexOf.If the counter says zero, there is no need to scan the slice. Great optimization.
188-190: Symmetry inLastIndexOf.Similar optimization to
IndexOf. Good for performance on large collections.
212-214: Prepending while updating counts.Prepending slices might reallocate internally. Thanks to the subsequent increment, the new values remain tracked properly.
Also applies to: 216-218
222-223: Reuse existing reduce helpers.Again, using
comfyReduceSlicestandardizes approach. This consistency fosters easier maintenance.
225-232: Return removed value fromRemoveAt.This is a user-friendly approach allowing callers to handle the removed value. Decrementing it in the counter is crucial for consistency.
235-245: Synchronized removal while updatingvc.Building a new slice and a new counter is a simple way to ensure no leftover references. This approach helps maintain internal consistency at the cost of reallocation—likely acceptable here.
Also applies to: 248-258
343-351: Cloning the sequence.The
copymethod returns a newcomfyCmpSeqwith a fresh counter—ensuring the original remains untouched. Good design for immutability patterns.base_cases_test.go (6)
9-11: Good transition tobaseInternal[int].These type aliases properly align with the
baseInternal[int]approach used throughout. No issues found.
13-15: Consistent naming for pair-based type aliases.Renaming the pair-based type aliases to use
baseInternal[Pair[int, int]]is consistent with the rest of the refactoring. Good job.
601-631: Extended coverage for duplicates in Find().The new
getFindCasesWithDupes()function covers scenarios with repeated pairs. This adds valuable test coverage for verifying that the first matching occurrence is returned. Nicely done.
633-643: Proper test harness for Find() with duplicates.
testFindWithDupes()ensures that the newly introduced duplicate-handling logic is tested thoroughly. No issues spotted.
706-736: Extended coverage for duplicates in FindLast().The new
getFindLastCasesWithDupes()function verifies correct behavior for the last matching occurrence with repeated pairs. Looks good.
738-748: Validating FindLast() with duplicates.
testFindLastWithDupes()offers comprehensive coverage for last-occurrence scenarios. Implementation appears correct.sequence_test.go (42)
8-8: Refactor to generic parameter for comfySeqIntBuilder.Using
[C baseInternal[int]]refines the builder’s generic constraint. This aligns with thebaseInternal[...]approach across the codebase.
12-12: Potential subtlety with returning a nil slice.Switching from
[]int{}to[]int(nil)for the empty slice is typically harmless in Go but can lead to subtle differences when comparing slices by pointer. Please verify that downstream code doesn’t rely on a non-nil empty slice.
27-31: Addition of ThreeRev() for reversed initialization.Providing a reversed sequence builder adds flexibility to tests and usage scenarios. Implementation looks consistent.
35-41: Generic FromValues() method improves test flexibility.Allowing appends from a slice of
anybroadens the builder’s utility. Code is straightforward and clear.
43-43: Updated make() signature.Defining
make(items []int) linearMutableInternal[int]clarifies the builder’s return type. No concerns.
51-57: New extraction helpers for raw and underlying slices.The methods
extractRawValuesandextractUnderlyingSliceneatly expose the internal slice for testing. Code matches naming conventions.
59-69: Nil returns in extractUnderlying methods.*
extractUnderlyingMap,extractUnderlyingKp, andextractUnderlyingValsCountcurrently returnnil. If these placeholders are intended for future expansions, that’s fine; otherwise, confirm their usage.
94-109: Increased test coverage for NewSequence().Ensuring non-nil and verifying the internal comfySeq structure. The checks for both
intandstringsequences look correct.
114-132: NewSequenceFrom() test coverage.The test verifies creation from
[]intand[]string. No issues spotted.
135-138: Appending one or many elements.Tests
testAppendOneandtestAppendManyconfirm correct behavior for single and bulk appends. Nicely done.
140-142: Appending collections.
testAppendCollchecks appending entire collections. Straightforward coverage.
145-146: Applying functions to each element.Valid coverage with
testApply. No concerns.
149-150: Index-based element access with testAt.Ensures
At()is correct. Implementation looks fine.
153-154: Default-based element access with testAtOrDefault.Zero-index edge cases are covered. Good approach.
157-158: Clearing the sequence.
testClearchecks full removal of items. Looks good.
161-162: Contains() test.Verifies correct predicate matching. Straightforward.
165-166: Count() test.Ensures correct counting logic with multiple conditions. No issues.
169-170: Each() iteration test.Simple iteration coverage. Approved.
173-174: Reverse iteration with EachRev().Confirmed coverage for iterating in reverse order.
177-178: Conditional reverse iteration with EachRevUntil().Verifies early-stop logic in reverse iteration.
181-182: Conditional forward iteration with EachUntil().Ensures early-stop logic in normal iteration.
185-187: Extended duplicate testing in Find().Including
testFindWithDupesbroadens coverage for repeated elements.
190-192: Extended duplicate testing in FindLast().Ensures correct retrieval of the last matching element.
195-196: Folding sequence elements.
testFoldchecks accumulation logic. Looks fine.
199-200: Head() test coverage.Confirms retrieval of the first element.
203-204: HeadOrDefault() test coverage.Ensures safe retrieval of the first element or default.
207-208: Index-based insertion.
testInsertAtchecks that items are inserted at the correct position.
211-212: Empty check.
testIsEmptycarefully verifies empty vs. non-empty sequences.
215-216: Len() coverage.Ensures the length is reported correctly.
219-221: Prepending items.
testPrependOneandtestPrependManyconfirm behavior at the front of the sequence.
224-225: Reduce() test coverage.Validates reduction logic with or without empty collection.
228-229: Removal by index.
testRemoveAtchecks correct removal at specified positions.
232-233: Removal by matching predicate.
testRemoveMatchingensures items meeting certain conditions are removed.
236-238: Reverse logic validation.
testReverseandtestReverseTwiceconfirm reversing the sequence multiple times.
241-243: Linear search coverage.
testSearchandtestSearchPairhandle normal search scenarios for single and pair-based sequences.
246-248: Reverse search coverage.
testSearchRevandtestSearchRevPairconfirm backward search.
251-252: Sort() coverage.Ensures items are sorted correctly.
255-256: Tail() test coverage.Checks retrieval of last element in the sequence.
259-260: TailOrDefault() test coverage.Ensures safe retrieval of the last element or default.
263-264: Converting to slice.
testToSlicechecks that sequences convert properly.
267-269: Breaking from Values() iteration.
testValuesandtestValuesBreakconfirm partial iteration logic.
272-273: Copying the sequence.
testCopyverifies a new sequence instance with equivalent data.sequencecmp_test.go (48)
13-13: Empty sequence creation returning a nil slice.Similar to
sequence_test.go, confirm that returning[]int(nil)is acceptable for your use cases.
28-31: ThreeRev() method for reverse order.Provides a reversed sequence for testing or usage. Approved.
36-42: FromValues() builder for comfyCmpSeq.Populates the sequence from arbitrary inputs. Straightforward approach.
44-46: Extracting raw values.
extractRawValuesdelegates toextractUnderlyingSlice, a clean separation of concerns.
48-50: Underlying slice extraction.Exposes
sfrom thecomfyCmpSeqfor testing. Straightforward.
52-58: Unimplemented extraction methods returning nil.
extractUnderlyingMapandextractUnderlyingKpcurrently returnnil. If these are placeholders, that’s fine; otherwise clarify their usage plans.
60-66: Extracting the values counter.Good introduction of
extractUnderlyingValsCount(). The panic is helpful to catch missing counters.
68-79: make() function with new values counter logic.Populating
vcviaIncrementensures accurate counts for later operations. Implementation looks correct.
86-87: Verifying default comfyCmpSeq is empty.Reflection-based check for
[]int(nil)plus a freshvaluesCounteris thorough.
94-95: String-based empty sequence check.Ensures
[]string(nil)plus an empty counter is also handled.
99-131: NewCmpSequenceFrom tests.Checks initialization of underlying slice and the values counter. Good demonstration of the new counting logic.
134-136: Appending elements.
testAppendOneandtestAppendManyconfirm appended items update bothsand the counter.
139-140: Appending collections.Ensures multiple items are appended and counted correctly.
143-144: Apply() coverage.Confirms the function is applied to each element properly.
147-148: At() coverage.Ensures correct index-based retrieval.
151-152: AtOrDefault() coverage.Checks safe retrieval for out-of-bounds.
155-156: Clear() coverage.Ensures clearing the sequence also resets the values counter.
159-160: Contains() coverage.Simple predicate-based contain check.
163-164: ContainsValue() coverage.Leverages the values counter for O(1) membership checks. Looks good.
167-168: Count() coverage.Verifies predicate-based counts. No issues.
171-172: CountValues() coverage.Ensures that the entire
valuesCounteris used to get counts.
175-176: Forward iteration with Each().Checks simple iteration logic. Approved.
179-180: Backward iteration with EachRev().Ensures correct reverse iteration.
183-184: EachRevUntil() coverage.Verifies early stopping in reverse iteration.
187-188: EachUntil() coverage.Ensures early stopping in normal iteration.
191-192: Find() coverage.Checks single match logic with the new counting approach.
195-196: FindLast() coverage.Validates last-match behavior.
198-200: Fold() coverage.Ensures accumulation logic works properly with counted sequences.
202-204: HasValue() coverage.Confirms membership checks using the
valuesCounter.
215-216: IndexOf() coverage.Ensures correct index discovery for existing elements.
218-233: IndexOf() invalid internal state test.Panic on mismatch between
vcandsis a solid safeguard. Great test.
247-262: LastIndexOf() invalid internal state test.Similar to
IndexOfInvalidState, verifying mismatch gracefully triggers a panic.
265-266: Len() coverage.Verifies length matches the slice size.
269-270: Max() coverage.Ensures correct maximum detection with the
cmpBaseInternalapproach.
273-274: Min() coverage.Similar approach to finding the minimum. Looks good.
277-279: Prepend coverage.Verifies adding elements at the beginning updates the sequence and counter.
281-283: Reduce coverage.Checks the reduce function on a counted sequence.
286-287: RemoveAt coverage.Ensures removing by index updates sequence and counter.
290-291: RemoveMatching coverage.Confirms removing items via predicate updates both structures.
298-300: Reverse coverage.
testReverseandtestReverseTwiceconfirm reversing. No issues.
303-304: Search coverage.Ensures standard searching logic works with the counting approach.
307-308: Reverse search coverage.
testSearchRevchecks backward searching.
311-312: Sort coverage.Ensures correct sorting with index-based logic.
315-316: Ascending sort coverage.
testSortAscverifies ascending ordering with comparators.
323-324: Sum coverage.
testSumensures numeric summation logic withcmpBaseInternal.
335-336: ToSlice coverage.Confirms correct slice extraction.
339-341: Values iteration coverage.
testValuesandtestValuesBreakconfirm iteration with possible early breaks.
344-345: Copy coverage.Ensures a newly allocated instance with matching data and counts.
cmpmutable_cases_test.go (25)
10-10: Generalization of builder type signature looks good.
This refactor to remove the second type parameter inorderedMutableCollIntBuilderhelps maintain a cleaner interface.
18-19: Improved test verification for empty collections.
Replacing[]int{}with[]int(nil)and adding awant2map is a neat approach to verify both the slice content and internal value counts for empty collections.
25-26: Consistent approach for single-item collection.
Great job ensuring that the newwant2verification is also applied for one-item cases.
33-33: Enhanced coverage for removal tests.
Includingwant2: map[int]int{222: 1, 333: 1}is helpful to confirm the internal count logic for partial removals.
40-40: Consistent test verification for second removal scenario.
Usingwant2: map[int]int{111: 1, 333: 1}aligns well with the new counting approach.
47-47: Removal test coverage is comprehensive.
Verifying removal of the third item withwant2: map[int]int{111: 1, 222: 1}ensures correctness.
54-54: Correctly verifying no removal.
want2: map[int]int{111: 1, 222: 1, 333: 1}underscores that the collection remains unchanged if the value isn't found.
61-61: Multiple duplicates removal test improved.
want2: map[int]int{222: 2, 333: 2}after removing two111s is an excellent validation of duplicate handling.
68-68: Increased clarity for removing repeated values.
Trackingwant2: map[int]int{111: 2, 333: 2}ensures that the correct duplicates are removed.
75-75: Ensuring multi-duplicate removal logic.
This test case for333duplicates further cements consistent behavior (want2: map[int]int{111: 2, 222: 2}).
85-91: Enhanced result verification intestRemoveValues.
Good job extracting both the raw slice (actualSlice) and the underlying values count (actualVC) to validate removal logic thoroughly.
97-97: Generics-based function signature for sorting tests.
Switching to afunc getSortAscCases[C any](...)approach increases reusability of the test builder.
99-103: Nil slice vs. empty map for empty collection.
Declaring bothwant1: []int(nil)andwant2: map[int]int{}ensures consistent representation of an empty state.
106-106: One-item collection test case logic is correct.
The transition to a generic test case structure provides uniform coverage for single-element scenarios.
110-110: Value count verification for single item.
want2: map[int]int{111: 1}demonstrates that sorting preserves item counts correctly.
113-113: Validating ascending sort with multiple items.
Explicitly enumerating[111, 222, 333]verifies correct sorting for standard ascending sequences.
117-117: Ensuring counts remain intact post-sort.
want2: map[int]int{111: 1, 222: 1, 333: 1}reconfirms that sorting didn’t alter duplicates count.
120-120: Testing sorting with duplicates.
want1: []int{111, 111, 222, 222, 333, 333}is a comprehensive approach for verifying stable sorting.
124-124: Confirmed correct value count for duplicates after sort.
want2remains consistent:{111: 2, 222: 2, 333: 2}.
127-131: Reversed collection sorting coverage.
Handling of reversed sequences is now validated withbuilder.ThreeRev()and verifying both slice and map viawant2.
134-138: Six-item reversed collection test.
The logic neatly checks for correct ascending order and ensures duplicates remain valid inwant2.
141-141: Return statement for test cases is straightforward.
Good to see them all grouped into a single slice.
151-151: Generic signature fortestSortAsc.
This change aligns sorting tests with the new internal constraints.
156-164: Detailed validation after SortAsc().
Extracting raw values and verifying the underlying values count fosters robust coverage.
168-220: Comprehensive descending sort test scenarios.
All lines ensure thorough coverage with reversed order, duplicates, etc.map.go (2)
5-425: Overall structural refactor from kvPair to Pair[K, V].
The conversion to storePair[K, V]acrosscomfyMapimproves clarity and consistency. The updated set/get logic and new methods (e.g.,RemoveAt) appear correct and cohesive.
63-65:Details
✅ Verification successful
Check interface usage for Pair return values.
Returningnilfor aPair[K, V]suggestsPairis an interface. Confirm that all usage sites handle potential nil values correctly.
🏁 Script executed:
#!/bin/bash # Search for references to At() usage to ensure nil-checks are in place. rg -A 5 'At\s*\('Length of output: 26713
Verified: Pair Interface Usage is Consistent and Safe
After checking across the codebase, we’ve confirmed that the
At()methods in bothmap.goand related implementations (e.g., inmapcmp.go) correctly returnnilwhen the index is out of bounds. The associated tests (in files likemap_cases_test.goandmap_test.go) verify that all usage sites properly handle the potential nil values returned for aPair[K, V].No further modifications are required.
functions_internal.go (3)
146-147: Min comparison logic not covered by tests.
The lines handling thecurrent < acccheck incomfyMinremain untested. Ensure you include a scenario that triggers a new minimum in your test suite.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 146-147: functions_internal.go#L146-L147
Added lines #L146 - L147 were not covered by tests
243-259: Summation logic introduced for ordered values.
comfySumandcomfySumOfPairsunify summation across collections. Implementation looks sound.
261-274:sliceRemoveAtenhancements are correct.
Returning the removed element and ensuring an out-of-bounds error fosters safer usage.definitions.go (13)
20-20: Parameter rename looks good.
Switching fromvtovalenhances clarity and consistency in function signatures.
23-23: Parameter rename looks good.
Again, usingvalinstead ofvimproves consistency.
29-29: Parameter rename looks good.
This is consistent with other mapper function signatures.
31-35: Introducing comparator types.
DefiningComparatorandPairComparatorclearly separates element comparison from key-value comparison, improving reusability.
41-41: Key-Value predicate signature.
The updated parameter names improve readability and align with the naming convention established.
64-67: Added forward and reverse search methods.
SearchandSearchRevprovide symmetrical lookup patterns. Ensure any references to the old search methods are updated, and incorporate appropriate tests if not already present.
75-78: BasePairs interface refinement.
EmbeddingBase[Pair[K, V]]clarifies that pairs remain the fundamental unit. This interface flattening helps maintain a consistent abstraction.
118-119: Enhanced IndexedMutable.
Adding(removed V, err error)return signature forRemoveAtand introducing aSortmethod fosters a more robust API for mutable indexed collections.
132-134: Index-based access for values.
HasValue,IndexOf, andLastIndexOfround out typical collection operations. Usage looks straightforward.
187-208: Comprehensive Map interface augmentation.
ComposingBasePairs,IndexedMutable, andLinearMutablebroadens map capabilities. Ensure that the newly added methods (e.g.,SetMany,RemoveMany) are consistently tested.
211-214: CmpMap interface extension.
Deriving fromMap[K, V]while addingCmpMutable[V]merges map operations with ordering semantics. This design clarifies typed constraints on map values.
221-222: Pair now supports value mutation.
Val()plusSetVal()fosters a more open approach to pair modifications. Ensure concurrency concerns (if any) are addressed.
226-231: NewPair constructor.
Straightforward constructor returning a handled implementation ofPair[K, V]. Good addition for standardizing pair creation.mapcmp.go (64)
8-8: Additional import for map utilities.
Importing"maps"can simplify operations likemaps.Collect(...).
13-20: New CmpMap creation.
The constructor initializes slices, maps, and thevaluesCounter, setting a foundation for powerful typed operations.
22-27: Factory method to build map from pairs.
NewCmpMapFromclarifies usage by letting clients provide pairs directly. Implementation referencessetMany, promoting code reuse.
30-34: Internal structure for comfyCmpMap.
Storing pairs in both a slice (s) and a map (m), pluskpfor indexing, appears well structured for combined index-based and hash-based lookups.
36-38: Append method.
Delegation tocomfyAppendMapretains consistent logic. Straightforward approach for adding new pairs.
40-42: AppendColl method.
Accepts anotherLinearcollection, converting it to a slice for append. Good extension for composability.
44-64: Apply method re-initialization.
Reconstructing the entire internal state ensures correct mapping results without partial updates. Performance is O(n), which is acceptable unless extremely large collections require more incremental updates.
66-71: At method returns nil for out-of-bounds.
ReturningnilforPair[K, V]when out of range might raise nil dereferences if callers forget thefoundcheck. Document thoroughly or provide a no-op pair to avoid potential runtime issues.
73-78: AtOrDefault method.
Simple fallback approach if index is out of range. This is consistent with typical "or default" patterns.
80-85: Clear method.
Re-initializes all fields, effectively resetting. Straightforward and easy to reason about.
87-88: Contains with a helper.
comfyContainsusage ensures consistency with the rest of the codebase’s utility approach.
91-95: Value-based containment check.
ContainsValuelogic is well aligned with the element-based approach inContains.
97-99: Count method.
Good usage of thecomfyCounthelper, consistent with parallel APIs.
101-105: CountValues for duplicates.
Allows direct counting of how many times a certain value appears. Minimal overhead beyond standard iteration.
107-110: Forward iteration with Each.
Direct iteration over the underlying slice. Implementation is straightforward.
113-116: Reverse iteration with EachRev.
Iterating in reverse is helpful for cases needing the last entry first. Clean approach.
119-125: Reverse iteration with early exit.
Provides a short-circuit mechanism. Be mindful that the condition might be inverted (stop on!f). Implementation is consistent with the forward variant.
127-133: Forward iteration with early exit.
Mirrors the logic inEachRevUntil. Behavior remains predictable.
135-137: Find method.
Fallback todefaultValueif not found. Clean helper usage ofcomfyFind.
139-141: FindLast method.
MirrorsFindlogic but in reverse. Symmetry is good for search-based operations.
143-144: Fold method.
Aggregates pairs in a manner consistent with typical functional fold.
147-154: Get method.
Retrieves from the map quickly while preserving pair semantics. Returningfalseif missing is standard.
156-162: GetOrDefault method.
Simple fallback ensures we return a user-specified default if the pair is absent.
164-167: Has key check.
A straightforward membership check that complementsGet.
169-171: HasValue delegates to ContainsValue.
This minor convenience function is consistent with existing patterns.
173-178: Head method returns nil for empty.
Similar toAt, returningnilwhen empty might lead to unguarded calls to.Key(). Ensure usage is well documented.
180-185: HeadOrDefault.
Gives a default pair when no head is present. Good approach for safe retrieval.
187-195: IndexOf method.
Detects the first occurrence of a value. Standard approach.
196-204: (Additional IndexOf logic).
Continues searching for duplicates in larger collections. Follows a consistent pattern.
196-198: IsEmpty method.
Zero-length check for slice-based collections.
200-208: KeyValues method.
Returns a lazy factory that yields key-value pairs. Alignment withiter.Seq2is consistent.
210-218: Keys method.
Similar lazy approach for keys only.
220-222: KeysToSlice method.
Builds on top ofKeys.slices.Collectusage is straightforward.
224-231: LastIndexOf method.
Searching from the end for a matching value. Matches forwardIndexOflogic.
233-235: Len method.
Returns slice length, minimal overhead.
237-239: Max function.
Uses the comfyMaxOfPairs helper. Integrates well with generic type constraints.
241-243: Min function.
Implemented similarly toMaxfor symmetrical behavior.
245-247: Prepend operation.
Defers logic to a dedicated method (prependAll). Good for consistency withAppend.
249-251: Reduce operation.
ReusescomfyReduceSlice, matching the approach used elsewhere.
253-255: Remove by key.
A direct call to internalremove. Straightforward single-key removal.
257-265: RemoveAt method.
LeveragessliceRemoveAt, updates maps, and decrements value count. Implementation is thorough.
267-269: RemoveMany method.
Extends the single-key removal logic. Useful for batch deletions.
271-292: RemoveMatching method.
Filters out pairs that match a predicate, re-allocating internal structures. Standard pattern for selective removal.
294-299: RemoveValues convenience.
Focuses on a single value match. Internally callsRemoveMatching.
300-309: Reverse method.
Reverses the slice in place, re-indexeskp. Implementation looks consistent.
311-318: Search returning nil for unfound pairs.
Same caveat asAt: ensure users always check the boolean before accessing the interface.
320-327: SearchRev reversing the search.
Consistent approach toSearch. Just note the same nil caveat if not found.
329-331: Set method.
Adds or replaces a key’s pair, updating the supporting data structures.
333-335: SetMany method.
Batch version ofSet; reusessetMany.
337-339: Sort method.
UsescomfySortSliceAndKPto reorder pairs. This is critical for maintaining a consistentkpindex post-sort.
341-349: SortAsc method.
Default ascending sort by pair values. Basic comparison logic is correct.
352-361: SortDesc method.
MirrorsSortAscwith reversed comparison. Keeps the code symmetrical.
363-365: Sum method.
Computes sum of values in the map, reusing a helper function. Straightforward.
367-372: Tail method returns nil if empty.
Analogous toHead. Same potential risk of nil usage if callers do not check.
374-379: TailOrDefault method.
Provides a default for empty maps, skipping nil.
381-383: ToMap method.
Builds a nativemap[K]V. The underlying iteration usesmaps.Collectfor convenience.
385-387: ToSlice method.
Collects pairs into a slice, aligning well withValues().
389-397: Values method.
Returns a lazy enumerator for content. This matches the approach used inKeyValues().
401-408: copy method.
Generates a new map instance by reusingsetlogic and creating fresh pair copies.
410-423: set method.
Handles insertion or replacement, adjusting counters and indexes. Code structure is direct.
425-455: prependAll method.
Works in two passes: first adds new pairs, then re-adds existing ones. Implementation is well-structured.
457-474: remove method.
Removes the pair from both slice and maps, carefully reindexing the remaining elements.
476-508: removeMany method.
Extends single-key removal while batching keys. Good approach to avoid repeated overhead in multiple remove calls.
510-514: setMany method.
Batched version ofset. Contributes to performance improvements when adding multiple pairs.cmp_cases_test.go (15)
8-47: Test cases for ContainsValue.
The variety of scenarios (empty, one-item, multi-item with duplicates) ensures robust coverage.
49-58: testContainsValue function.
Iterates over cases, validating correctness. Straightforward usage of table-driven tests.
60-69: testHasValue duplicates testContainsValue logic.
Consistent with the library’s naming patterns. The reuse ensures alignment between analogous methods.
71-128: getCountValuesCases.
Covers single, multiple, and zero occurrences. Thorough for verifyingCountValues.
130-140: testCountValues function.
Prints both result and wanted outcome. Keep or remove the error messages for clarity.
142-210: getIndexOfCases.
Drills edge cases, including empty collections and duplicates. Good coverage.
212-225: testIndexOf function.
Checks both index and boolean. Fulfills thorough testing for partial or absent values.
227-293: getLastIndexOfCases.
MirrorsgetIndexOfCaseswith reversed logic. Good parallel structure.
295-308: testLastIndexOf function.
Similar approach totestIndexOf, verifying last match. Straightforward.
310-334: getMaxCases.
Tests extremes (empty, single, multi). Accuracy forMaxis well validated.
336-350: testMax function.
Ensures error handling for empty input and correct detection of the highest value.
352-376: getMinCases.
Parallel coverage forMin. Looks consistent withgetMaxCases.
378-392: testMin function.
MirrorstestMaxfor verifying minimum detection.
394-417: getSumCases.
Examines summation across various collection sizes and duplicates.
419-429: testSum function.
Uses table-driven tests to validate the sum of items, including empty and multi-item scenarios.map_test.go (2)
103-117: The test structure looks goodThis test validates that
NewMap[int, int]()returns a properly initialized map with all required internal structures.
328-364: Good test for deep copy behaviorThis test effectively verifies that
copy()creates both a new instance (pointer equality) and a deep copy (modifying the original doesn't affect the copy).mapcmp_test.go (3)
111-126: Test structure matches collection implementationThe test properly validates that
NewCmpMap[int, int]()returns a correctly initialized map with all internal structures, including the values counter.
325-327: Good test coverage for value-specific operationsTests like
Test_comfyCmpMap_RemoveValuescover important functionality specific to the enhanced map implementation. This shows good test coverage for the new features.
337-339: Comprehensive test suiteThe test file provides excellent coverage of the new map implementation's functionality, including both standard map operations and the additional comparison features.
| func (c *valuesCounter[V]) Decrement(v V) { | ||
| if c.counter[v] == 1 { | ||
| delete(c.counter, v) | ||
| } else { | ||
| c.counter[v]-- | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add check for key existence in Decrement method
The current implementation assumes the key exists in the map before checking if its count is 1. If a key doesn't exist, accessing it will return 0, causing incorrect behavior.
func (c *valuesCounter[V]) Decrement(v V) {
- if c.counter[v] == 1 {
+ count, exists := c.counter[v]
+ if !exists {
+ return
+ }
+ if count == 1 {
delete(c.counter, v)
} else {
c.counter[v]--
}
}📝 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.
| func (c *valuesCounter[V]) Decrement(v V) { | |
| if c.counter[v] == 1 { | |
| delete(c.counter, v) | |
| } else { | |
| c.counter[v]-- | |
| } | |
| } | |
| func (c *valuesCounter[V]) Decrement(v V) { | |
| count, exists := c.counter[v] | |
| if !exists { | |
| return | |
| } | |
| if count == 1 { | |
| delete(c.counter, v) | |
| } else { | |
| c.counter[v]-- | |
| } | |
| } |
list_cases_test.go
Outdated
| }, | ||
| { | ||
| name: "InsertAt() on one-item collection at beginning", | ||
| coll: builder.One(), | ||
| args: listIntArgs{index: 0, value: 1}, | ||
| want1: []int{1, 111}, | ||
| want2: map[int]int{1: 1, 111: 1}, | ||
| }, |
There was a problem hiding this comment.
Added duplicate test case for insertion at the beginning
This test case appears to be a duplicate of the one at lines 22-28, as both test "InsertAt() on one-item collection at beginning" with identical inputs and expected outputs.
Consider removing this duplicated test case, or if it's intended to test a different edge case, rename it to clarify the distinction.
| c1 := &comfyCmpSeq[int]{ | ||
| s: []int{123, 234, 345}, | ||
| vc: &valuesCounter[int]{ | ||
| counter: map[int]int{ | ||
| 123: 1, | ||
| 234: 1, | ||
| 345: 1, | ||
| }, | ||
| }, | ||
| } | ||
|
|
There was a problem hiding this comment.
Logic error in the deep copy test loop.
for v := range c2.(*comfyCmpSeq[int]).s {
if v == 999 {
t.Error("copy() did not create a deep copy")
}
}Again, v here is the index, not the actual element. Use:
for i, val := range c2.(*comfyCmpSeq[int]).s {
if val == 999 {
t.Error(...)
}
}to check elements correctly.
Also applies to: 368-369
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmp.go (2)
15-20: Consider simplifying the Count method.While the current implementation is correct, Go maps return the zero value (0 for int) for non-existent keys, so you could potentially simplify this to just
return c.counter[v]. However, the explicit check does make the code's intent clearer, so this is more of a stylistic suggestion.func (c *valuesCounter[V]) Count(v V) int { - if _, ok := c.counter[v]; !ok { - return 0 - } - return c.counter[v] + return c.counter[v] }
22-28: Consider simplifying the Increment method.Similar to the Count method, you could potentially simplify this by leveraging Go's zero value behavior for maps. However, the explicit check does make the code's intent clearer.
func (c *valuesCounter[V]) Increment(v V) { - if _, ok := c.counter[v]; !ok { - c.counter[v] = 1 - } else { - c.counter[v]++ - } + c.counter[v]++ }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmp.go(1 hunks)list_cases_test.go(3 hunks)sequencecmp_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- list_cases_test.go
- sequencecmp_test.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
cmp.go
[warning] 33-34: cmp.go#L33-L34
Added lines #L33 - L34 were not covered by tests
🔇 Additional comments (2)
cmp.go (2)
1-7: Well-structured generic type declaration.The
valuesCountertype is properly defined with a generic parameterVconstrained tocmp.Ordered, which ensures values can be compared and used as map keys. This is a good design choice for a counter implementation.
9-13: Constructor implementation looks good.The constructor function properly initializes the counter map. The lowercase function name indicates it's package-private, which is appropriate if this type is meant to be used only internally within the package.
| func (c *valuesCounter[V]) Decrement(v V) { | ||
| count, exists := c.counter[v] | ||
| if !exists { | ||
| return | ||
| } | ||
| if count == 1 { | ||
| delete(c.counter, v) | ||
| } else { | ||
| c.counter[v]-- | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Add test coverage for the non-existent key case in Decrement.
The implementation correctly checks for key existence before attempting to decrement, which addresses the issue raised in the previous review. However, static analysis indicates that lines 33-34 (the early return when a key doesn't exist) aren't covered by tests.
Please add test cases that verify the behavior when attempting to decrement a non-existent key to ensure complete test coverage.
🏁 Script executed:
#!/bin/bash
# Check if there are any existing tests that might cover this code path
rg -A 5 -B 5 "Decrement" --glob "**/*_test.go"Length of output: 46
Action Required: Add Test Case for Non-Existent Key in Decrement
The Decrement method in cmp.go (lines 30–40) handles the non-existent key case by returning early when the key isn't found. However, our repository lacks tests covering this branch (as indicated by the absence of test output for references to Decrement in *_test.go files). Please add a unit test that calls Decrement on a key not present in the counter to verify that it exits gracefully without modifying the counter.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 33-34: cmp.go#L33-L34
Added lines #L33 - L34 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
cmp_test.go (4)
5-35: Good test for valuesCounter but could be improved with table-driven testingThe test covers key functionality, including edge cases like decrementing before incrementing (line 8) and checking for non-existent values (line 31-33). However, consider using a table-driven test approach to reduce repetition in the assertion blocks.
func Test_valuesCounter(t *testing.T) { t.Run("Increment() and Count()", func(t *testing.T) { vc := newValuesCounter[int]() vc.Decrement(1) vc.Increment(1) vc.Increment(1) vc.Increment(2) vc.Increment(3) vc.Increment(3) vc.Increment(3) vc.Increment(4) vc.Increment(4) vc.Increment(4) vc.Increment(4) - if vc.Count(1) != 2 { - t.Errorf("Count(1) = %v, want %v", vc.Count(1), 2) - } - if vc.Count(2) != 1 { - t.Errorf("Count(2) = %v, want %v", vc.Count(2), 1) - } - if vc.Count(3) != 3 { - t.Errorf("Count(3) = %v, want %v", vc.Count(3), 3) - } - if vc.Count(4) != 4 { - t.Errorf("Count(4) = %v, want %v", vc.Count(4), 4) - } - if vc.Count(5) != 0 { - t.Errorf("Count(5) = %v, want %v", vc.Count(5), 0) - } + testCases := []struct { + value int + expected int + }{ + {1, 2}, + {2, 1}, + {3, 3}, + {4, 4}, + {5, 0}, // Non-existent value + } + + for _, tc := range testCases { + t.Run(fmt.Sprintf("Count(%d)", tc.value), func(t *testing.T) { + if count := vc.Count(tc.value); count != tc.expected { + t.Errorf("Count(%d) = %v, want %v", tc.value, count, tc.expected) + } + }) + } }) }Remember to add
"fmt"to the imports if you implement this refactoring.
6-6: Update subtest name to reflect all tested methodsThe subtest name only mentions "Increment() and Count()", but the test also exercises the Decrement() method on line 8.
- t.Run("Increment() and Count()", func(t *testing.T) { + t.Run("Increment(), Decrement(), and Count()", func(t *testing.T) {
6-34: Consider adding separate subtests for each methodThe current approach tests multiple methods in a single test. Consider restructuring to test each method separately for better isolation and clearer failure messages.
You could split this into separate subtests:
- A test for basic Increment() and Count() behavior
- A test specifically for Decrement() behavior
- A test for the zero/default behavior (e.g., counting non-existent values)
This would make the test more maintainable and easier to debug if failures occur.
8-18: Consider documenting the test pattern or setup with commentsThe test performs a specific sequence of operations, but it's not immediately clear why these exact operations were chosen. Adding a brief comment to explain the test setup would improve readability.
func Test_valuesCounter(t *testing.T) { t.Run("Increment() and Count()", func(t *testing.T) { + // Create counter and test: + // - Decrementing a non-existent value + // - Multiple increments with different frequencies + // - Counting both existing and non-existing values vc := newValuesCounter[int]() vc.Decrement(1) vc.Increment(1) vc.Increment(1) // ...rest of the code
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmp.go(1 hunks)cmp_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmp.go
1 participant
Summary by CodeRabbit
Documentation
New Features
Refactor
Tests
valuesCounterfunctionality to validate counting operations.Chores