Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add WithoutBy #515

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

nicklaus-dev
Copy link

see #505

@@ -176,6 +176,17 @@ func Without[T comparable, Slice ~[]T](collection Slice, exclude ...T) Slice {
return result
}

// WithoutBy returns a slice excluding values whose extracted key is in the exclude list.
func WithoutBy[T any, K comparable](collection []T, extract func(item T) K, exclude ...K) []T {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can also the function def in README.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I fixed it. Please review again.

intersect.go Outdated
@@ -176,6 +176,17 @@ func Without[T comparable, Slice ~[]T](collection Slice, exclude ...T) Slice {
return result
}

// WithoutBy returns a slice excluding values whose extracted key is in the exclude list.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I modifed this comments. Please review it again.

@@ -270,6 +270,26 @@ func TestWithout(t *testing.T) {
is.IsType(nonempty, allStrings, "type preserved")
}

func TestWithoutBy(t *testing.T) {
t.Parallel()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
    }
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a intersect_example_test.go file to add this example. Please review it again.

README.md Outdated


```go
type user {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, it's a little bit misleading. I fixed it in README.md and test files. Please review it again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing the changes. It's clearer now.

Copy link
Contributor

@shivamrazorpay shivamrazorpay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nicklaus-dev
Copy link
Author

All comments have been resolved. Can you reivew this PR again? @samber

README.md Outdated


```go
type User {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: missing struct here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I fixed it, please review it again.

intersect.go Outdated
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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@senago senago Aug 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...)
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have refactored WithoutBy function. I used blacklist to replace whitelist for code readability. It's clear now. Please review it again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, with blacklist it's simpler
I suggested whitelist cause this way we can preallocate the exact capacity for the resulting slice

@nicklaus-dev
Copy link
Author

There are some improvements of WithoutBy and Without, please review it again. @ccoVeille @senago @shivamrazorpay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants