Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1608)

Issue 4973055: code review 4973055: exp/sql{,/driver}: new database packages (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 9 months ago by bradfitz
Modified:
12 years, 9 months ago
Reviewers:
CC:
gustavo_niemeyer.net, rsc, borman, dave_cheney.net, kevlar, nigeltao, dvyukov, kardia, fw, r, r2, crawshaw2, golang-dev
Visibility:
Public.

Description

exp/sql{,/driver}: new database packages

Patch Set 1 #

Patch Set 2 : diff -r f6449df27a7f https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r f6449df27a7f https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r f6449df27a7f https://go.googlecode.com/hg/ #

Total comments: 19

Patch Set 5 : diff -r 4beaeaead995 https://go.googlecode.com/hg #

Patch Set 6 : diff -r 4beaeaead995 https://go.googlecode.com/hg #

Patch Set 7 : diff -r 7469b572b79b https://go.googlecode.com/hg #

Total comments: 52

Patch Set 8 : diff -r 9a44dedca5dd https://go.googlecode.com/hg #

Patch Set 9 : diff -r 9a44dedca5dd https://go.googlecode.com/hg #

Total comments: 45

Patch Set 10 : diff -r 93c459658010 https://go.googlecode.com/hg #

Patch Set 11 : diff -r 6bfb65b2ff19 https://go.googlecode.com/hg #

Patch Set 12 : diff -r 6bfb65b2ff19 https://go.googlecode.com/hg #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1837 lines, -0 lines) Patch
M src/pkg/Makefile View 1 2 3 4 5 6 7 2 chunks +3 lines, -0 lines 0 comments Download
A src/pkg/exp/sql/Makefile View 1 2 3 4 1 chunk +12 lines, -0 lines 0 comments Download
A src/pkg/exp/sql/convert.go View 1 2 3 4 5 6 7 8 9 10 1 chunk +106 lines, -0 lines 0 comments Download
A src/pkg/exp/sql/convert_test.go View 1 2 3 4 5 6 7 8 9 10 1 chunk +108 lines, -0 lines 0 comments Download
A src/pkg/exp/sql/doc.txt View 1 2 3 4 5 6 7 8 9 1 chunk +46 lines, -0 lines 0 comments Download
A src/pkg/exp/sql/driver/Makefile View 1 2 3 4 1 chunk +12 lines, -0 lines 0 comments Download
A src/pkg/exp/sql/driver/driver.go View 1 2 3 4 5 6 7 8 9 10 1 chunk +169 lines, -0 lines 0 comments Download
A src/pkg/exp/sql/driver/types.go View 1 2 3 4 5 6 7 8 9 10 1 chunk +161 lines, -0 lines 0 comments Download
A src/pkg/exp/sql/fakedb_test.go View 1 2 3 4 5 6 7 8 9 10 1 chunk +497 lines, -0 lines 0 comments Download
A src/pkg/exp/sql/sql.go View 1 2 3 4 5 6 7 8 9 10 1 chunk +578 lines, -0 lines 0 comments Download
A src/pkg/exp/sql/sql_test.go View 1 2 3 4 5 6 7 8 9 10 1 chunk +145 lines, -0 lines 0 comments Download

Messages

Total messages: 44
bradfitz
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
12 years, 9 months ago (2011-09-02 17:04:13 UTC) #1
bradfitz
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), Please take another look.
12 years, 9 months ago (2011-09-02 17:04:26 UTC) #2
bradfitz
Docs at: http://p.danga.com:6060/pkg/exp/db/ http://p.danga.com:6060/pkg/exp/db/dbimpl/ Goals of the db and db/dbimpl packages: * Provide a generic ...
12 years, 9 months ago (2011-09-02 17:05:39 UTC) #3
bradfitz
Lost in the original CL description after a server error: This was designed with Russ ...
12 years, 9 months ago (2011-09-02 17:07:10 UTC) #4
gustavo_niemeyer.net
Hey Brad, > * Provide a generic database API for a variety of SQL or ...
12 years, 9 months ago (2011-09-02 17:23:23 UTC) #5
bradfitz
On Fri, Sep 2, 2011 at 10:23 AM, Gustavo Niemeyer <gustavo@niemeyer.net>wrote: > Hey Brad, > ...
12 years, 9 months ago (2011-09-02 17:29:44 UTC) #6
rsc
Is there any hope of a more general package called db? If not, then db ...
12 years, 9 months ago (2011-09-02 17:31:34 UTC) #7
gustavo_niemeyer.net
> Is there any hope of a more general package called db? Having nothing more ...
12 years, 9 months ago (2011-09-02 17:46:15 UTC) #8
bradfitz
On Fri, Sep 2, 2011 at 10:45 AM, Gustavo Niemeyer <gustavo@niemeyer.net>wrote: > > Is there ...
12 years, 9 months ago (2011-09-02 17:52:24 UTC) #9
borman
I agree. I think something like sql, sqldb, or dbsql. This CL makes me long ...
12 years, 9 months ago (2011-09-02 17:52:35 UTC) #10
bradfitz
On Fri, Sep 2, 2011 at 10:23 AM, Gustavo Niemeyer <gustavo@niemeyer.net>wrote: > Hey Brad, > ...
12 years, 9 months ago (2011-09-02 20:00:24 UTC) #11
gustavo_niemeyer.net
> Latest thinking: > sql > sql/driver (maybe; better than sql/sqlimpl) > Then it's *sql.DB ...
12 years, 9 months ago (2011-09-03 00:08:10 UTC) #12
dave_cheney.net
+1 Sent from my iPad On 03/09/2011, at 6:00, Brad Fitzpatrick <bradfitz@golang.org> wrote: > On ...
12 years, 9 months ago (2011-09-03 00:15:43 UTC) #13
kevlar
I like the idea of this. While I don't think it's necessarily incompatible with some ...
12 years, 9 months ago (2011-09-03 01:26:10 UTC) #14
nigeltao
On 3 September 2011 03:04, <bradfitz@golang.org> wrote: > Description: > exp/db{,/dbimpl}: new database packages > ...
12 years, 9 months ago (2011-09-03 01:49:16 UTC) #15
bradfitz
(replies to some comments below; others later) On Fri, Sep 2, 2011 at 6:49 PM, ...
12 years, 9 months ago (2011-09-03 02:02:00 UTC) #16
nigeltao
On 3 September 2011 12:01, Brad Fitzpatrick <bradfitz@golang.org> wrote: >> It seems odd that most ...
12 years, 9 months ago (2011-09-03 02:12:52 UTC) #17
nigeltao
On 3 September 2011 12:12, Nigel Tao <nigeltao@golang.org> wrote: > I know that it's deliberate. ...
12 years, 9 months ago (2011-09-03 02:37:43 UTC) #18
bradfitz
On Fri, Sep 2, 2011 at 7:37 PM, Nigel Tao <nigeltao@golang.org> wrote: > On 3 ...
12 years, 9 months ago (2011-09-03 03:17:22 UTC) #19
dvyukov
http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go File src/pkg/exp/db/db.go (right): http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode35 src/pkg/exp/db/db.go:35: func Register(name string, driver dbimpl.Driver) { If it's not ...
12 years, 9 months ago (2011-09-03 10:17:53 UTC) #20
kardia
Your Rows type is more of a Cursor or Iter then a Rows struct. For ...
12 years, 9 months ago (2011-09-03 20:19:41 UTC) #21
rsc
On Fri, Sep 2, 2011 at 22:37, Nigel Tao <nigeltao@golang.org> wrote: > A stronger statement ...
12 years, 9 months ago (2011-09-04 16:45:46 UTC) #22
fw
* Brad Fitzpatrick: > * To type casting/conversions consistently between all drivers. To > achieve ...
12 years, 9 months ago (2011-09-05 04:58:12 UTC) #23
nigeltao
On 3 September 2011 13:17, Brad Fitzpatrick <bradfitz@golang.org> wrote: > The point of making them ...
12 years, 9 months ago (2011-09-05 11:32:17 UTC) #24
nigeltao
On 3 September 2011 11:49, Nigel Tao <nigeltao@golang.org> wrote: > // Yeah, it's actually a ...
12 years, 9 months ago (2011-09-07 01:47:33 UTC) #25
rsc
I think you are reasoning backwards. The goal is to have a nice, easy-to-use API, ...
12 years, 9 months ago (2011-09-07 02:05:03 UTC) #26
nigeltao
On 7 September 2011 12:04, Russ Cox <rsc@golang.org> wrote: > The goal is to have ...
12 years, 9 months ago (2011-09-07 02:48:23 UTC) #27
bradfitz
http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go File src/pkg/exp/db/db.go (right): http://codereview.appspot.com/4973055/diff/7001/src/pkg/exp/db/db.go#newcode61 src/pkg/exp/db/db.go:61: Ok bool // Ok is true if the String ...
12 years, 9 months ago (2011-09-16 00:04:42 UTC) #28
bradfitz
Hello golang-dev@googlegroups.com, gustavo@niemeyer.net, rsc@golang.org, borman@google.com, dave@cheney.net, kevlar@google.com, nigeltao@golang.org, dvyukov@google.com, kardianos@gmail.com, fw@deneb.enyo.de (cc: golang-dev@googlegroups.com), Please take ...
12 years, 9 months ago (2011-09-16 00:04:44 UTC) #29
r
http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go File src/pkg/exp/sql/sql.go (right): http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newcode45 src/pkg/exp/sql/sql.go:45: // NullableString is type representing a string which may ...
12 years, 9 months ago (2011-09-16 00:12:07 UTC) #30
bradfitz
On Thu, Sep 15, 2011 at 5:12 PM, <r@golang.org> wrote: > > http://codereview.appspot.com/**4973055/diff/28001/src/pkg/** > exp/sql/sql.go#newcode61<http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newcode61> ...
12 years, 9 months ago (2011-09-16 00:14:11 UTC) #31
r2
On Sep 15, 2011, at 5:14 PM, Brad Fitzpatrick wrote: > On Thu, Sep 15, ...
12 years, 9 months ago (2011-09-16 00:16:07 UTC) #32
bradfitz
Valid it is. There are a few thousand other lines to review too. On Thu, ...
12 years, 9 months ago (2011-09-16 00:23:43 UTC) #33
r2
I don't know SQL worth a damn but I'll look it over for second-order stuff.
12 years, 9 months ago (2011-09-16 01:45:43 UTC) #34
crawshaw2
http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go File src/pkg/exp/sql/sql.go (right): http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/sql.go#newcode92 src/pkg/exp/sql/sql.go:92: freeConn []driver.Conn So you are maintaining an underlying connection ...
12 years, 9 months ago (2011-09-16 04:16:57 UTC) #35
rsc
Only looking at public API. More comments in driver would help. My main concern right ...
12 years, 9 months ago (2011-09-16 15:17:57 UTC) #36
bradfitz
http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/driver.go File src/pkg/exp/sql/driver/driver.go (right): http://codereview.appspot.com/4973055/diff/28001/src/pkg/exp/sql/driver/driver.go#newcode38 src/pkg/exp/sql/driver/driver.go:38: // The dsn parameter, the Data Source Name, contains ...
12 years, 9 months ago (2011-09-29 01:08:55 UTC) #37
bradfitz
Hello gustavo@niemeyer.net, rsc@golang.org, borman@google.com, dave@cheney.net, kevlar@google.com, nigeltao@golang.org, dvyukov@google.com, kardianos@gmail.com, fw@deneb.enyo.de, r@golang.org, r@google.com, david.crawshaw@zentus.com (cc: golang-dev@googlegroups.com), ...
12 years, 9 months ago (2011-09-29 01:09:09 UTC) #38
r
LGTM within my limited understanding. there's some nice stuff in here. the interfaces are somewhat ...
12 years, 9 months ago (2011-09-29 01:28:37 UTC) #39
bradfitz
All done. Russ, thoughts on this as a checkpoint? http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/convert.go File src/pkg/exp/sql/convert.go (right): http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/convert.go#newcode28 src/pkg/exp/sql/convert.go:28: ...
12 years, 9 months ago (2011-09-29 18:43:25 UTC) #40
bradfitz
Hello gustavo@niemeyer.net, rsc@golang.org, borman@google.com, dave@cheney.net, kevlar@google.com, nigeltao@golang.org, dvyukov@google.com, kardianos@gmail.com, fw@deneb.enyo.de, r@golang.org, r@google.com, david.crawshaw@zentus.com (cc: golang-dev@googlegroups.com), ...
12 years, 9 months ago (2011-09-29 18:43:38 UTC) #41
r
http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/driver.go File src/pkg/exp/sql/driver/driver.go (right): http://codereview.appspot.com/4973055/diff/43001/src/pkg/exp/sql/driver/driver.go#newcode86 src/pkg/exp/sql/driver/driver.go:86: LastInsertId() (int64, os.Error) insertion goes into the gaps. is ...
12 years, 9 months ago (2011-09-29 23:01:46 UTC) #42
rsc
LGTM Fix copyright notices.
12 years, 9 months ago (2011-09-29 23:02:15 UTC) #43
bradfitz
12 years, 9 months ago (2011-09-29 23:12:31 UTC) #44
*** Submitted as http://code.google.com/p/go/source/detail?r=d21caa7909f2 ***

exp/sql{,/driver}: new database packages

R=gustavo, rsc, borman, dave, kevlar, nigeltao, dvyukov, kardianos, fw, r, r,
david.crawshaw
CC=golang-dev
http://codereview.appspot.com/4973055
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b