feat: adding "mutable" subpackage by samber · Pull Request #482 · samber/lo

samber · 2024-06-30T01:24:57Z

Looking for feedback on this PR.

codecov-commenter · 2024-06-30T01:25:06Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.48%. Comparing base (9405c90) to head (2f6fef8).

❗ 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

Flag	Coverage Δ
unittests	`94.48% <100.00%> (+0.11%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

samber · 2024-06-30T01:26:33Z

mutable/slice.go

+// 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) {


Compared to immutable package, I chose to split Filter into 2 helpers: the I means predicate receives an index as parameter.

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

@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.

samber · 2024-06-30T01:28:01Z

slice.go

 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)


I just figured out that lo.Shuffle is having a side effect.

samber · 2024-06-30T01:28:38Z

slice.go

 // 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.


Deprecating Reverse that will be moved to samber/lo/mutable package

samber · 2024-06-30T01:30:31Z

mutable/slice.go

@@ -0,0 +1,95 @@
+package mutable


package mutable

or

package lom

?

It does matters, because it will be the default package alias on import.

No one will get lom, mutable as a package is good.

Don't enforce your import alias as package name 😅😬

samber · 2024-06-30T01:49:47Z

mutable/slice.go

+// 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) {


replace with lo/internal/rand (PR is WIP)

Elbehery · 2024-06-30T08:04:01Z

mutable/slice.go

+	j := 0
+	for i := range *collection {
+		if predicate((*collection)[i], i) {
+			(*collection)[j] = (*collection)[i]


iiuc, this is immutable, so I think you should dump the filtered elems into a new collection, or ?

nope ;)

this subpackage would be dedicated to mutable helpers

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

I'm not sure to understand. Do you expect predicate() could manipulate collection ?

PS: don't worry 😅✊

(*collection)[j] = (*collection)[i]

mutable/slice_test.go

ccoVeille · 2024-06-30T19:57:40Z

mutable/slice.go

+// 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) {


But slices.Compact exists

So your code is somehow about using this no?

foo := slices.Compact(collections)
*collection = *foo

yes there is already a stdlib version of this， don't add it.

ccoVeille · 2024-06-30T19:58:46Z

mutable/slice.go

+// 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) {


This is slices.CompactFunc followerd by

*collection = *foo

ccoVeille · 2024-06-30T19:59:54Z

mutable/slice.go

+
+// 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) {


https://pkg.go.dev/slices#Reverse

trim21 · 2024-06-30T21:53:21Z

mutable/slice.go

+
+// Shuffle returns an array of shuffled values. Uses the Fisher-Yates shuffle algorithm.
+// Play:
+func Shuffle[T any](collection []T) {


I would suggest to not add this function, it's just save 2 lines saver alias for stdlib.

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.

Add a // Deprecated note then, if you don't want them anymore

feat: adding "mutable" subpackage

2f6fef8

samber commented Jun 30, 2024

View reviewed changes

Elbehery reviewed Jun 30, 2024

View reviewed changes

mutable/slice_test.go Show resolved Hide resolved

ccoVeille reviewed Jun 30, 2024

View reviewed changes

trim21 reviewed Jun 30, 2024

View reviewed changes

Uh oh!

Conversation

samber commented Jun 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jun 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

samber Jun 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fdegiuli Sep 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samber Jun 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trim21 Jun 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

samber commented Jun 30, 2024 •

edited

Loading

codecov-commenter commented Jun 30, 2024 •

edited

Loading

samber Jun 30, 2024 •

edited

Loading

fdegiuli Sep 30, 2024 •

edited

Loading

samber Jun 30, 2024 •

edited

Loading

trim21 Jun 30, 2024 •

edited

Loading