[feat] add map Values function; allow CombineSlices to take no args (BREAKING CHANGE) #10
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new map helper function (Values) and refactors the CombineSlices function to allow being called with no arguments while enhancing its test coverage.
- Updates the CombineSlices function to accept variadic slice inputs and adjust its logic accordingly.
- Introduces additional tests in slice_test.go for various input scenarios for CombineSlices.
- Adds a new Values function in map.go to retrieve map values.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| slice_test.go | Updates tests for CombineSlices with additional and refined cases. |
| slice.go | Refactors CombineSlices to support no arguments and to avoid unnecessary copies. |
| map_test.go | Removes extraneous semicolons for stylistic consistency. |
| map.go | Adds a new Values function to extract map values. |
Comments suppressed due to low confidence (1)
slice.go:83
- [nitpick] Consider renaming the parameter 'more' to 'slices' to more clearly indicate that all provided slices are being combined. This might aid readability for new callers.
func CombineSlices[T any](more ...[]T) []T {
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 3
Lines 169 172 +3
=========================================
+ Hits 169 172 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
muir
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new Values function for maps and updates the CombineSlices function to allow no arguments (with explicit generic binding required).
- Adds unit tests for the new Values function in map_test.go
- Refactors CombineSlices in slice.go and updates its tests in slice_test.go to cover multiple edge cases, including nil returns and optimizations
- Minor stylistic adjustments in test cases
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| slice_test.go | Updated tests for CombineSlices with additional edge case scenarios |
| slice.go | Refactored CombineSlices to support variadic slice inputs and optimizations |
| map_test.go | New test cases for the Values function and minor style adjustments |
| map.go | Added implementation for the Values function |
Comments suppressed due to low confidence (2)
slice.go:83
- [nitpick] Consider renaming the variadic parameter 'more' to 'slices' or similar to better convey that it represents all input slices.
func CombineSlices[T any](more ...[]T) []T {
map.go:11
- [nitpick] Consider adding a function comment for Values to document its behavior, similar to the documentation provided for the Keys function.
func Values[K comparable, V any](m map[K]V) []V {
6208f9e to
08bea92
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new map utility function, Values, and refactors CombineSlices to accept a variadic number of slices (including no slices), with the latter being a breaking change.
- Introduces a new Values function in map.go along with comprehensive tests in map_test.go.
- Updates CombineSlices in slice.go to support zero or one generic slice input, and adjusts the associated tests in slice_test.go accordingly.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| slice_test.go | Updated test cases for CombineSlices, covering various input cases |
| slice.go | Refactored CombineSlices to accept variadic slices with breaking change |
| map_test.go | Added tests ensuring correct behavior of the new Values function |
| map.go | Implemented the new Values function for map utilities |
slice.go
Outdated
| if total == len(first) { | ||
| return first | ||
| if total == len(slices[0]) { | ||
| return slices[0] |
There was a problem hiding this comment.
Returning the original slice when no additional slices contribute elements may lead to unintended side effects if the caller later mutates the result. Consider documenting this behavioral nuance or copying the slice to enforce immutability.
| return slices[0] | |
| return CopySlice(slices[0]) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
agree, looking at the tests this seems to be expected, but maybe worth documenting?
There was a problem hiding this comment.
Added a CombineSlicesCopy that always copies. Documented.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces two main features: a new map Values function to extract values from a map, and updates to the CombineSlices functionality to allow a call with no arguments (with an explicit generic binding required in that scenario).
- Added Values function and associated tests in map.go and map_test.go.
- Refactored CombineSlices and added CombineSlicesCopy in slice.go, along with comprehensive tests in slice_test.go to verify various slice combination behaviors.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| slice_test.go | Updated tests for CombineSlices and added tests for CombineSlicesCopy behavior |
| slice.go | Modified CombineSlices to accept variadic slices and added CombineSlicesCopy |
| map_test.go | Added tests for the new Values function |
| map.go | Added simple implementation of the Values function |
slice.go
Outdated
| // CombineSlices may return the first slice if it is the only slice with elements. A copy | ||
| // is only made if it has to be made. | ||
| func CombineSlices[T any](first []T, more ...[]T) []T { | ||
| if len(more) == 0 { | ||
| return first | ||
| } | ||
| if len(first) == 0 && len(more) == 1 { | ||
| return more[0] | ||
| // is only made if it has to be made. For no input, nil is returned. | ||
| func CombineSlices[T any](slices ...[]T) []T { | ||
| switch len(slices) { | ||
| case 0: | ||
| return nil | ||
| case 1: | ||
| return slices[0] | ||
| case 2: | ||
| if len(slices[0]) == 0 { | ||
| return slices[1] | ||
| } |
There was a problem hiding this comment.
[nitpick] The special handling for exactly two slices—returning the second slice when the first is empty—may lead to inconsistent behavior compared to cases with more slices. Consider evaluating if this optimization should be extended or clearly documented to avoid confusion.
Copilot uses AI. Check for mistakes.
slice.go
Outdated
| return CombineSlicesCopy[T](slices...) | ||
| } | ||
|
|
||
| // CombineSlicesCopy combines all of it's input slices, always |
There was a problem hiding this comment.
[nitpick] Consider using 'its' instead of 'it's' in the comment for grammatical correctness.
| // CombineSlicesCopy combines all of it's input slices, always | |
| // CombineSlicesCopy combines all of its input slices, always |
Copilot uses AI. Check for mistakes.
3 participants
The Values function is obvious.
CombineSlices now all can have no arguments, though in that case, the generic binding must be provided explicitly.