feat: adding "mutable" subpackage #482
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #482 +/- ##
==========================================
+ Coverage 94.36% 94.48% +0.11%
==========================================
Files 17 18 +1
Lines 2664 2719 +55
==========================================
+ Hits 2514 2569 +55
Misses 148 148
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| // The function returns the modified slice, which may be shorter than the original if some elements were removed. | ||
| // The order of elements in the original slice is preserved in the output. | ||
| // Play: | ||
| func FilterI[T any](collection *[]T, predicate func(item T, index int) bool) { |
There was a problem hiding this comment.
Compared to immutable package, I chose to split Filter into 2 helpers: the I means predicate receives an index as parameter.
There was a problem hiding this comment.
I would not use the I name and use your current FilterI as Filter
There is an index, I don't use it, let's user ignore it. You have no need to provide a variant of all your methods
There was a problem hiding this comment.
@ccoVeille I disagree, I think this is an excellent change.
There is almost no use for the I version of the methods, especially for operations such as map and filter.
The extra index parameter makes it impossible to use existing methods (e.g. strings.ToLower) as iteratees, which forces most usage of these methods to unnecessarily verbose and hard to parse.
Splitting them into separate methods does mess with autocomplete, but the usability gain IMO is well worth it.
| @@ -273,15 +273,21 @@ func Interleave[T any, Slice ~[]T](collections ...Slice) Slice { | |||
| // Shuffle returns an array of shuffled values. Uses the Fisher-Yates shuffle algorithm. | |||
| // Play: https://go.dev/play/p/Qp73bnTDnc7 | |||
| func Shuffle[T any, Slice ~[]T](collection Slice) Slice { | |||
| rand.Shuffle(len(collection), func(i, j int) { | |||
| collection[i], collection[j] = collection[j], collection[i] | |||
| size := len(collection) | |||
There was a problem hiding this comment.
I just figured out that lo.Shuffle is having a side effect.
| } | ||
|
|
||
| // Reverse reverses array so that the first element becomes the last, the second element becomes the second to last, and so on. | ||
| // Play: https://go.dev/play/p/fhUMLvZ7vS6 | ||
| // | ||
| // Deprecated: Use lom.Reverse instead. |
There was a problem hiding this comment.
Deprecating Reverse that will be moved to samber/lo/mutable package
| @@ -0,0 +1,95 @@ | |||
| package mutable | |||
There was a problem hiding this comment.
package mutableor
package lom
?
It does matters, because it will be the default package alias on import.
There was a problem hiding this comment.
No one will get lom, mutable as a package is good.
Don't enforce your import alias as package name 😅😬
| // Shuffle returns an array of shuffled values. Uses the Fisher-Yates shuffle algorithm. | ||
| // Play: | ||
| func Shuffle[T any](collection []T) { | ||
| rand.Shuffle(len(collection), func(i, j int) { |
There was a problem hiding this comment.
replace with lo/internal/rand (PR is WIP)
| j := 0 | ||
| for i := range *collection { | ||
| if predicate((*collection)[i], i) { | ||
| (*collection)[j] = (*collection)[i] |
There was a problem hiding this comment.
iiuc, this is immutable, so I think you should dump the filtered elems into a new collection, or ?
There was a problem hiding this comment.
nope ;)
this subpackage would be dedicated to mutable helpers
There was a problem hiding this comment.
ok, it is more safe to work on a copied variable, since the iteration might mutate the collection and this could cause subtle hidden bugs iiuc
P.S. dont vote to the right :D
There was a problem hiding this comment.
I'm not sure to understand. Do you expect predicate() could manipulate collection ?
PS: don't worry 😅✊
| // Uniq returns a duplicate-free version of an array, in which only the first occurrence of each element is kept. | ||
| // The order of result values is determined by the order they occur in the array. | ||
| // Play: | ||
| func Uniq[T comparable](collection *[]T) { |
There was a problem hiding this comment.
But slices.Compact exists
So your code is somehow about using this no?
foo := slices.Compact(collections)
*collection = *foo
There was a problem hiding this comment.
yes there is already a stdlib version of this， don't add it.
| // The order of result values is determined by the order they occur in the array. It accepts `iteratee` which is | ||
| // invoked for each element in array to generate the criterion by which uniqueness is computed. | ||
| // Play: | ||
| func UniqBy[T any, U comparable](collection *[]T, iteratee func(item T) U) { |
There was a problem hiding this comment.
This is slices.CompactFunc followerd by
*collection = *foo
|
|
||
| // Reverse reverses array so that the first element becomes the last, the second element becomes the second to last, and so on. | ||
| // Play: | ||
| func Reverse[T any](collection []T) { |
There was a problem hiding this comment.
|
|
||
| // Shuffle returns an array of shuffled values. Uses the Fisher-Yates shuffle algorithm. | ||
| // Play: | ||
| func Shuffle[T any](collection []T) { |
There was a problem hiding this comment.
I would suggest to not add this function, it's just save 2 lines saver alias for stdlib.
There was a problem hiding this comment.
The point of adding this function here is to deprecate it from parent package.
2y ago, I added 2 functions with mutable behavior (Shuffle + Reverse). It was an error.
There was a problem hiding this comment.
Add a // Deprecated note then, if you don't want them anymore
Looking for feedback on this PR.