feat: add WithoutBy#515
Conversation
| return result | ||
| } | ||
|
|
||
| // WithoutBy returns a slice excluding values whose extracted key is in the exclude list. |
There was a problem hiding this comment.
This is can be a much better comment I think:
// WithoutBy filters a slice by excluding elements whose extracted keys match any in the exclude list.
// It returns a new slice containing only the elements whose keys are not in the exclude list.
There was a problem hiding this comment.
Thanks, I modifed this comments. Please review it again.
| } | ||
|
|
||
| func TestWithoutBy(t *testing.T) { | ||
| t.Parallel() |
There was a problem hiding this comment.
Can also add this in the test , This gives a more real life example:
func main() {
// Example usage
users := []User{
{ID: 1, Name: "Alice"},
{ID: 2, Name: "Bob"},
{ID: 3, Name: "Charlie"},
}
// Exclude users with IDs 2 and 3
excludedIDs := []int{2, 3}
// Extract function to get the user ID
extractID := func(user User) int {
return user.ID
}
// Filtering users
filteredUsers := WithoutBy(users, extractID, excludedIDs...)
// Output the filtered users
for _, user := range filteredUsers {
fmt.Println(user.Name)
}
}
There was a problem hiding this comment.
I added a intersect_example_test.go file to add this example. Please review it again.
|
|
||
|
|
||
| ```go | ||
| type user { |
There was a problem hiding this comment.
I'm a bit suspicious about the fact the struct is unexported in the example. But also that the fields are unexported.
Based on the extract func, it might work, but I'm unsure, the README should not promote using unexported struct with unexported field.
The _test is in the same package, so it's not a proof it works
There was a problem hiding this comment.
OK, it's a little bit misleading. I fixed it in README.md and test files. Please review it again.
There was a problem hiding this comment.
Thanks for doing the changes. It's clearer now.
|
All comments have been resolved. Can you reivew this PR again? @samber |
|
|
||
|
|
||
| ```go | ||
| type User { |
There was a problem hiding this comment.
nitpick: missing struct here
There was a problem hiding this comment.
OK. I fixed it, please review it again.
| func WithoutBy[T any, K comparable](collection []T, extract func(item T) K, exclude ...K) []T { | ||
| result := make([]T, 0, len(collection)) | ||
| for _, item := range collection { | ||
| if !Contains(exclude, extract(item)) { |
There was a problem hiding this comment.
I'd say it's worth using sets here for better performance
func WithoutBy[T any, K comparable](collection []T, extract func(item T) K, exclude ...K) []T {
whitelist := make(map[K]struct{}, len(collection))
for _, item := range collection {
whitelist[extract(item)] = struct{}{}
}
for _, k := range exclude {
delete(whitelist, k)
}
result := make([]T, 0, len(whitelist))
for _, item := range collection {
if _, ok := whitelist[extract(item)]; ok {
result = append(result, item)
}
}
return result
}Might as well refactor existing function Without
There was a problem hiding this comment.
And maybe It would be good to simply call WithoutBy in Without, most likely compiler will optimize such a call (could check in godbolt)
func Without[T comparable](collection []T, exclude ...T) []T {
return WithoutBy(collection, func(item T) T { return item }, exclude...)
}There was a problem hiding this comment.
I have refactored WithoutBy function. I used blacklist to replace whitelist for code readability. It's clear now. Please review it again.
There was a problem hiding this comment.
Yeah, with blacklist it's simpler
I suggested whitelist cause this way we can preallocate the exact capacity for the resulting slice
|
There are some improvements of |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #515 +/- ##
==========================================
+ Coverage 94.12% 94.14% +0.02%
==========================================
Files 18 18
Lines 3046 3057 +11
==========================================
+ Hits 2867 2878 +11
Misses 168 168
Partials 11 11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
see #505