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

you don't need to implement Execer and Queryer #210

Closed
wants to merge 1 commit into from

Conversation

freeeve
Copy link

@freeeve freeeve commented Dec 16, 2013

Not sure if this is worth a PR, but you don't need to implement these. They're implemented in database/sql. Effectively, that code in conn.go will never be hit. I tweaked the unit test to check for Execer--it wasn't really testing anything before.

Exec:
http://golang.org/src/pkg/database/sql/sql.go?s=22844:22913#L836

Query:
http://golang.org/src/pkg/database/sql/sql.go?s=23894:23963#L885

Also, apparently my go fmt autorun when exit caught a few things.

@johto
Copy link
Contributor

johto commented Dec 16, 2013

Not sure if this is worth a PR, but you don't need to implement these. They're implemented in database/sql. Effectively, that code in conn.go will never be hit.

Huh? DB.Exec() calls exec(), which does this:

   867      if execer, ok := dc.ci.(driver.Execer); ok {
   868          dargs, err := driverArgs(nil, args)
   869          if err != nil {
   870              return nil, err
   871          }
   872          dc.Lock()
   873          resi, err := execer.Exec(query, dargs)

I'm not sure I have personal experience with Exec(), but I can tell you for a fact that our Queryer implementation is being called (by DB.queryConn). In fact, we want to implement these interfaces because we have more control over what happens in the database. See for example the simpleExec optimization, the use of the unnamed prepared statement and #209.

I tweaked the unit test to check for Execer--it wasn't really testing anything before.

It's testing that we implement the driver.Execer interface. I don't see why you think that that's not worth testing.

Also the exact same functionality your changes test is already tested by TestIssue186.

@freeeve
Copy link
Author

freeeve commented Dec 16, 2013

Cool, thanks for the explanation.

@freeeve freeeve closed this Dec 16, 2013
@freeeve freeeve deleted the unnecessary-interfaces branch December 16, 2013 09:45
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.

2 participants