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

database/sql: NamedParam and Param are wrong names #18099

Closed
rsc opened this issue Nov 29, 2016 · 11 comments
Closed

database/sql: NamedParam and Param are wrong names #18099

rsc opened this issue Nov 29, 2016 · 11 comments
Assignees
Milestone

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Nov 29, 2016

If you have

func f(x int) 

y = f(42)

then x is a function parameter and 42 is a function argument.

The new API sql.NamedParam and sql.Param are using the term parameter when they should be using argument. This is very confusing and it even contradicts the names used in the current docs: Exec takes args ...interface{} not params ...interface{}.

I suggest changing the type to sql.NamedArg and the constructor to sql.Named. Also sql.Named needs a good example. It can be short, but something like

db.Exec("delete from db where Time < ?end and Time >= ?start", sql.Named("start", startTime), sql.Named("end", endTime))

would help.

@rsc rsc added this to the Go1.8 milestone Nov 29, 2016
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Nov 29, 2016

@kardianos
Copy link
Contributor

@kardianos kardianos commented Nov 29, 2016

SGTM

@kardianos
Copy link
Contributor

@kardianos kardianos commented Nov 29, 2016

I'll create an example shortly.

I've made this change in a local CL, but one thing I noticed that everywhere else they are referred to as "Parameters" from DefaultParameterConverter to comments. There are two places where we reference something called "args", in the comment above the function // The args are for any placeholder parameters in the query. and in the function argument itself. I think the proper fix is to change the function argument variable to params and then we would be consistent.

In MS-SQL it universally references Parameter for SQL function input values:
https://technet.microsoft.com/en-us/library/ms177436(v=sql.105).aspx

In sqlite, it uses argument for the C API and parameter for the SQL arguments: https://www.sqlite.org/c3ref/bind_blob.html

In postgresql documentation, argument and parameter are both used: https://www.postgresql.org/docs/9.4/static/xfunc-sql.html

I don't care strongly, but I think because we are already using Parameter in parts of the exported API already we should change args to params.
@rsc

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 29, 2016

CL https://golang.org/cl/33660 mentions this issue.

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 29, 2016

CL https://golang.org/cl/33663 mentions this issue.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Nov 29, 2016

I mailed both a rename per @rsc and just a comment and argument rename per my suggestion. Take the one you'd like.

@quentinmit
Copy link
Contributor

@quentinmit quentinmit commented Nov 29, 2016

"named parameter" is the only name used for these in every SQL implementation out there... I think calling them "named arguments" will be very confusing for users. sql.Named sounds like a fine name for the constructor, though.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Nov 29, 2016

@rsc, you cool with struct named sql.Param, constructor func sql.Named and renaming func args from args to params?

@rsc
Copy link
Contributor Author

@rsc rsc commented Nov 30, 2016

The "?name" syntax in the SQL string is a parameter. The value passed to Query or Exec is an argument. During the execution of the SQL statement, the argument is bound to the parameter.

@quentinmit, without a citation to what you are talking about, it's very hard to respond usefully. All I will say is that yes, "named parameter" is a widespread (and accurate) term for the concept of the named placeholders in the SQL queries. But the Go value being passed to Query or Exec that is bound to the named parameter during the SQL execution is a named argument.

@kardianos, thanks for the links. The MS-SQL docs are using parameter correctly. They use "value" where I am suggesting using "argument". The sqlite docs are using parameter and argument correctly. The postgresql docs also seem to be using parameter and argument correctly (they're very long and I didn't read the whole page).

We should be precise, like all the other implementations are precise. The value passed to Query/Exec is an argument, not a parameter. The struct should be NamedArg, or NamedArgument, certainly not NamedParameter. It could reasonably be NamedValue but that's in driver and I wasn't sure if that would be too confusing.

@gopherbot gopherbot closed this in 2b1abf7 Nov 30, 2016
@gopherbot
Copy link

@gopherbot gopherbot commented Dec 1, 2016

CL https://golang.org/cl/33760 mentions this issue.

gopherbot pushed a commit that referenced this issue Dec 1, 2016
Updates #18099

Change-Id: I16b4b2dd881d63cbb406d14a4fd960f0a777a452
Reviewed-on: https://go-review.googlesource.com/33760
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@mattn
Copy link
Member

@mattn mattn commented Dec 1, 2016

Sorry posting comment on closed issue. @kardianos you've better to update this too: https://docs.google.com/document/d/1F778e7ZSNiSmbju3jsEWzShcb8lIO4kDyfKDNm4PNd8/edit#

mattn added a commit to mattn/go-sqlite3 that referenced this issue Dec 1, 2016
fix test
mattn added a commit to mattn/go-oci8 that referenced this issue Dec 1, 2016
fix test
@golang golang locked and limited conversation to collaborators Dec 1, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.