removed simple+incorrect IN expansion

[truesilver92]

in sql_builder.go line 91 already calls out to sqlx.In to do an expansion that supports multiple IN statements. The removed section duplicated behavior and has a bug between the regex selection and what strings.Replace replaces (selects \(\s*\?\s*\) but only replaces \(\?\)

[truesilver92]

This constitutes a breaking change. I think it is worth it, but I am not sure if you want to have a deprication time that warns about the change in behavior, and wait until at least next major release.

There is a work around for people that want the behavior that RawQuery and everything else currently has. Call sqlx.In passing your sql and arguments to do the expansion, then pass the resulting sql and arguments to Where

you can use compile's usage as an example https://github.com/gobuffalo/pop/blob/master/sql_builder.go#L91

[sio4]

Hello @truesilver92, thanks for your contribution!

The description of the PR looks nice but the patch is somewhat odd to me. I will take care of the code tomorrow and will update the result. Also, I am not sure if you want to work on this PR now. Please let me know if you have interest in it, otherwise, I will take it over and will fix it.

[sio4]

Although the direction of supporting slice as an argument for IN could be nice (as introduced by [4] for the first time), this PR cannot be merged for several reasons:

in sql_builder.go line 91 already calls out to sqlx.In to do an expansion that supports multiple IN statements. The removed section duplicated behavior

Roughly, they look the same, but we need to see them in a logical context. The "removed section" basically do a similar job, but the target statement is totally different. In the SQL Builder, the function takes a whole generated SQL statement so the statement could have multiple ?, so matching them with multiple arguments is not easy (a single value or slice per each ? could help this though). Previously we had a related issue [2] and PR [3] to fix it, and this change will bring that issue again.

and has a bug between the regex selection and what strings.Replace replaces (selects (\s*?\s*) but only replaces (?)

This is a valid point, I would like to file a new PR for this to fix this bug.

This constitutes a breaking change. I think it is worth it, but I am not sure if you want to have a deprication time that warns about the change in behavior, and wait until at least next major release.

Yes, this could be a big breaking change since .Where() could be one of the most popular methods for users, and they mostly use it with variadic arguments. Also, if we change this way completely, there are more things to be fixed with it to make it works.

There is a work around for people that want the behavior that RawQuery and everything else currently has. Call sqlx.In passing your sql and arguments to do the expansion, then pass the resulting sql and arguments to Where

Since the method still working smoothly (except for the regex bug), it could be fine if we do not change the current behavior of parsing the statement one more time here.

Anyway, I would like to keep this idea for the next major release.

Some references:

• [1] 5e912b0f9b21a3bdffcc0aa30dec3901c394995b support "in" queries
• [2] #65
• [3] #67
• [4] #267