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

proposal: improve handling positional parameters in WHERE #777

Open
sio4 opened this issue Sep 23, 2022 · 0 comments
Open

proposal: improve handling positional parameters in WHERE #777

sio4 opened this issue Sep 23, 2022 · 0 comments
Assignees
Labels
breaking change This feature / fix introduces breaking changes f: sql about SQL support proposal A suggestion for a change, feature, enhancement, etc s: hold This PR shouldn't be closed, but should wait for further work.
Milestone

Comments

@sio4
Copy link
Member

sio4 commented Sep 23, 2022

Consider fixing the behavior of positional argument handling. supporting both variadic arguments and slice with values is very confusing and it can be confused to users. Possible directions (without deep consideration) are:

  • match the number of ? and number of arguments and allow slices
  • support named arguments
  • creating another helpful method for IN to reduce complexity and easy usage

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.

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.

Some references:

Originally posted by @sio4 in #524 (comment)

@sio4 sio4 mentioned this issue Sep 23, 2022
30 tasks
@sio4 sio4 self-assigned this Sep 23, 2022
@sio4 sio4 added breaking change This feature / fix introduces breaking changes proposal A suggestion for a change, feature, enhancement, etc s: hold This PR shouldn't be closed, but should wait for further work. labels Sep 23, 2022
@sio4 sio4 added this to the v7.0.0 milestone Sep 23, 2022
@sio4 sio4 added the f: sql about SQL support label Sep 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This feature / fix introduces breaking changes f: sql about SQL support proposal A suggestion for a change, feature, enhancement, etc s: hold This PR shouldn't be closed, but should wait for further work.
Projects
None yet
Development

No branches or pull requests

1 participant