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

Add ability to use WhereIn with any slice type #673

Closed
wants to merge 3 commits into from

Conversation

tonyhb
Copy link

@tonyhb tonyhb commented Feb 21, 2020

Fixes #227 by using reflect to automatically convert any concrete slice type to a slice of interfaces where passed.

See #227 (comment) for more info.

In summary, this PR allows us to:

ids := []int{1, 2, 3, 4}
qm := WhereIn("id in ?", ids)

Verses before when we needed to:

ids := []int{1, 2, 3, 4}

slice := make([]interface{}, len(ids))
for index, num := range ids {
        slice[index] = num
}

qm := WhereIn("id in ?", slice...)

@tonyhb tonyhb changed the base branch from master to dev February 21, 2020 18:59
@tonyhb
Copy link
Author

tonyhb commented Feb 21, 2020

Note that, yes, this does now mean we have reflection as a dependency. However, reflection is only used if the len of args == 1, and only then if the first element is of type slice. Splatting your variadic parameter avoids all reflection and should not incur any speed penalties :)

@tonyhb tonyhb requested a review from aarondl February 21, 2020 21:35
@aarondl
Copy link
Member

aarondl commented Mar 6, 2020

This is an interesting change. On the one hand it solves a common problem that people have with the library (that expanding the variadic []int they have breaks because it wants []interface{} and Go is too restrictive). On the other hand it introduces a sort of weirdness around the types involved. Ordinarily if you saw a function signature like this you'd say: "Oh, I can do 1,2,3 or I can do []int{1,2,3}... as an argument. But now the frictionless way is to do: []int{1,2,3}.

I'm not sure what to make of it. It solves the pain, but only if you know about this little trick that sort of flies in the face of the language's normal conventions and would rely on documentation/closing issues pointing to said documentation to solve.

The other question is: How necessary is this given that this common use case is eliminated completely by models.UserWhere.ID.IN([]int{1,2,3}...)?

@tonyhb
Copy link
Author

tonyhb commented Mar 9, 2020

That's a good question and perhaps I need to put up a PR for generic types. By default, sqlboiler doesn't generate an IN helper for custom types (I use UUIDs from github.com/google/uuid for primary keys).

This means that I always need to convert a slice of UUIDs to the slice of interfaces in order to query via an IN right now.

@tonyhb
Copy link
Author

tonyhb commented Mar 9, 2020

Side note: I appreciate your thoughtful replies to everybody's PRs!

@aarondl
Copy link
Member

aarondl commented Mar 13, 2020

Currently in our code we also use UUIDs but we rely on postgres to do the validation largely for us. The UUIDs inside the objects themselves are strings so we're able to use the string helpers so we haven't run into this.

I'm not sure if you're comfortable with that approach because technically it could be any old string and type safety etc. However I do think that we will not go ahead with this change as it was just a bit too clever :)

I'm opened to some more interesting WhereIn generators for different types. Not sure what we could do exactly but maybe there's some possibilities there?

I'm going to close this PR, but I do appreciate the effort you put in to open it and the discussion it prompted. Thanks for that!

@aarondl aarondl closed this Mar 13, 2020
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.

Question passing an Int slice into WhereIn
2 participants