Issue 14087043: code review 14087043: database/sql: Improve package documentation
|
Created:
10 years, 6 months ago by mattcottingham Modified:
10 years, 6 months ago CC:
golang-dev, kisielk, adg, r, rsc, dave_cheney.net, ahormann, bradfitz Visibility:
Public. |
Descriptiondatabase/sql: Improve package documentation
Adds implementation-independent documentation
for connecting to and querying databases.
Fixes issue 5886.
Patch Set 1 #Patch Set 2 : diff -r 67b829e5a2a8 https://code.google.com/p/go #Patch Set 3 : diff -r 67b829e5a2a8 https://code.google.com/p/go #
Total comments: 8
Patch Set 4 : diff -r 67b829e5a2a8 https://code.google.com/p/go #Patch Set 5 : diff -r 67b829e5a2a8 https://code.google.com/p/go #Patch Set 6 : diff -r 67b829e5a2a8 https://code.google.com/p/go #
Total comments: 8
Patch Set 7 : diff -r 24ea55b48f9f https://code.google.com/p/go #Patch Set 8 : diff -r 24ea55b48f9f https://code.google.com/p/go #
Total comments: 2
Patch Set 9 : diff -r 24ea55b48f9f https://code.google.com/p/go #Patch Set 10 : diff -r 24ea55b48f9f https://code.google.com/p/go #Patch Set 11 : diff -r d2cb80eac1ac https://code.google.com/p/go #Patch Set 12 : diff -r d2cb80eac1ac https://code.google.com/p/go #
Total comments: 2
Patch Set 13 : diff -r 35d5bae6aac8 https://code.google.com/p/go #Patch Set 14 : diff -r 35d5bae6aac8 https://code.google.com/p/go #MessagesTotal messages: 41
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
https://codereview.appspot.com/14087043/diff/5001/src/pkg/database/sql/doc.go File src/pkg/database/sql/doc.go (right): https://codereview.appspot.com/14087043/diff/5001/src/pkg/database/sql/doc.go... src/pkg/database/sql/doc.go:18: Open is used to open a database: maybe this should say something other than "open a database" if in fact it doesn't (deferred...)?
Sign in to reply to this message.
https://codereview.appspot.com/14087043/diff/5001/src/pkg/database/sql/doc.go File src/pkg/database/sql/doc.go (right): https://codereview.appspot.com/14087043/diff/5001/src/pkg/database/sql/doc.go... src/pkg/database/sql/doc.go:36: result, err:= db.Exec( Maybe describe what the purpose of result is, if no rows are returned? https://codereview.appspot.com/14087043/diff/5001/src/pkg/database/sql/doc.go... src/pkg/database/sql/doc.go:42: Query or QueryRow (for queries expected to return one row) are used Maybe separate the description of QueryRow from Query since the code for dealing with the result is different. https://codereview.appspot.com/14087043/diff/5001/src/pkg/database/sql/doc.go... src/pkg/database/sql/doc.go:60: Prepared statements can also be created with Prepare: s/also//, then maybe mention what methods you can call on them?
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, kamil.kisiel@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/14087043/diff/5001/src/pkg/database/sql/doc.go File src/pkg/database/sql/doc.go (right): https://codereview.appspot.com/14087043/diff/5001/src/pkg/database/sql/doc.go... src/pkg/database/sql/doc.go:18: Open is used to open a database: On 2013/09/29 03:21:45, nathany wrote: > maybe this should say something other than "open a database" if in fact it > doesn't (deferred...)? Done. https://codereview.appspot.com/14087043/diff/5001/src/pkg/database/sql/doc.go... src/pkg/database/sql/doc.go:36: result, err:= db.Exec( On 2013/09/29 05:37:12, kisielk wrote: > Maybe describe what the purpose of result is, if no rows are returned? Done. https://codereview.appspot.com/14087043/diff/5001/src/pkg/database/sql/doc.go... src/pkg/database/sql/doc.go:42: Query or QueryRow (for queries expected to return one row) are used On 2013/09/29 05:37:12, kisielk wrote: > Maybe separate the description of QueryRow from Query since the code for dealing > with the result is different. Done. https://codereview.appspot.com/14087043/diff/5001/src/pkg/database/sql/doc.go... src/pkg/database/sql/doc.go:60: Prepared statements can also be created with Prepare: On 2013/09/29 05:37:12, kisielk wrote: > s/also//, then maybe mention what methods you can call on them? Done.
Sign in to reply to this message.
https://codereview.appspot.com/14087043/diff/18001/src/pkg/database/sql/doc.go File src/pkg/database/sql/doc.go (right): https://codereview.appspot.com/14087043/diff/18001/src/pkg/database/sql/doc.g... src/pkg/database/sql/doc.go:18: Open is used to create a database handle (type *DB): I think the type can be omitted here, it doesn't add anything to the description and can be looked up in the API. https://codereview.appspot.com/14087043/diff/18001/src/pkg/database/sql/doc.g... src/pkg/database/sql/doc.go:22: Where driver specifies the database driver to use (see above), and (see above) can be removed I think. Also the driver string depends on the driver being used but is not necessarily the package name. eg: github.com/lib/pq is used by specifying "postgres" as the driver. https://codereview.appspot.com/14087043/diff/18001/src/pkg/database/sql/doc.g... src/pkg/database/sql/doc.go:42: Where result is a Result type which contains the last insert ID and number of result is a value, not a type. Maybe "result is a value of the Result type". You should probably also note that the availability of the last insert id and rows affected value is database driver-dependant. https://codereview.appspot.com/14087043/diff/18001/src/pkg/database/sql/doc.g... src/pkg/database/sql/doc.go:75: defer stmt.Close() Not sure if this is a good pattern to encourage. Normally you'd keep a prepared statement around for a while so you can reuse it.
Sign in to reply to this message.
Thanks for the review. PTAL https://codereview.appspot.com/14087043/diff/18001/src/pkg/database/sql/doc.go File src/pkg/database/sql/doc.go (right): https://codereview.appspot.com/14087043/diff/18001/src/pkg/database/sql/doc.g... src/pkg/database/sql/doc.go:18: Open is used to create a database handle (type *DB): On 2013/10/01 05:53:38, kisielk wrote: > I think the type can be omitted here, it doesn't add anything to the description > and can be looked up in the API. Done. https://codereview.appspot.com/14087043/diff/18001/src/pkg/database/sql/doc.g... src/pkg/database/sql/doc.go:22: Where driver specifies the database driver to use (see above), and On 2013/10/01 05:53:38, kisielk wrote: > (see above) can be removed I think. Done. > Also the driver string depends on the driver being used but is not necessarily the package name. eg: github.com/lib/pq is > used by specifying "postgres" as the driver. I don't think this implies any particular format (since, as you mention, it's up to the driver), but I can re-word it if it's unclear. https://codereview.appspot.com/14087043/diff/18001/src/pkg/database/sql/doc.g... src/pkg/database/sql/doc.go:42: Where result is a Result type which contains the last insert ID and number of On 2013/10/01 05:53:38, kisielk wrote: > result is a value, not a type. Maybe "result is a value of the Result type". You > should probably also note that the availability of the last insert id and rows > affected value is database driver-dependant. Done. https://codereview.appspot.com/14087043/diff/18001/src/pkg/database/sql/doc.g... src/pkg/database/sql/doc.go:75: defer stmt.Close() On 2013/10/01 05:53:38, kisielk wrote: > Not sure if this is a good pattern to encourage. Normally you'd keep a prepared > statement around for a while so you can reuse it. It already says below that Close should be used, so I'll remove this.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, kamil.kisiel@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
LGTM other than the spacing fix. Someone from the Go team should probably pick it over now. https://codereview.appspot.com/14087043/diff/37001/src/pkg/database/sql/doc.go File src/pkg/database/sql/doc.go (right): https://codereview.appspot.com/14087043/diff/37001/src/pkg/database/sql/doc.g... src/pkg/database/sql/doc.go:36: result, err:= db.Exec( missing a space between err and :
Sign in to reply to this message.
https://codereview.appspot.com/14087043/diff/37001/src/pkg/database/sql/doc.go File src/pkg/database/sql/doc.go (right): https://codereview.appspot.com/14087043/diff/37001/src/pkg/database/sql/doc.g... src/pkg/database/sql/doc.go:36: result, err:= db.Exec( On 2013/10/01 18:21:05, kisielk wrote: > missing a space between err and : Done.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, kamil.kisiel@gmail.com, adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
This comment is too long, and putting large code snippets in comments is poor design because they cannot be automatically verified. They should be in executable examples instead.
Sign in to reply to this message.
On 2013/10/01 19:33:08, r wrote: > This comment is too long, and putting large code snippets in comments is poor > design because they cannot be automatically verified. They should be in > executable examples instead. I've been using https://code.google.com/p/go/source/browse/src/pkg/net/http/doc.go as a guide which has similar code listings. How should I go about making these executable examples? Move them to doc/progs with example output?
Sign in to reply to this message.
Looks like there already some examples in example_test.go Could probably flesh out some more of them by using code from these docs, and then maybe provide one larger package-level example?
Sign in to reply to this message.
There's an open bug to fix the documentation for package net. Please don't use it as a style guide. -rob
Sign in to reply to this message.
On 2013/10/01 21:17:09, r wrote: > There's an open bug to fix the documentation for package net. Please don't > use it as a style guide. > > -rob Thanks for clarifying -- I didn't see the ticket.
Sign in to reply to this message.
On 2013/10/01 21:02:25, kisielk wrote: > Looks like there already some examples in example_test.go > > Could probably flesh out some more of them by using code from these docs, and > then maybe provide one larger package-level example? Thanks for the pointer, I'll add the new examples and reduce the size of doc.go.
Sign in to reply to this message.
We had a chat about this CL today. Here's what we think: 1) Brad Fitzpatrick is away for a while, and we really want his input before this CL lands. 2) The CL as it stands, although it doesn't match what we want package docs to look like, contains excellent information. Therefore we propose: 1) Put this info on the wiki. 2) For now, just put a link to the wiki in the docs with a small update. Then when Brad returns we can figure out what we really want. We don't need to sort this out for the 1.2 timeline. Sound OK? -rob
Sign in to reply to this message.
In any case, making examples is always welcome. -rob
Sign in to reply to this message.
On 2013/10/01 23:34:01, r wrote: > We had a chat about this CL today. Here's what we think: > > 1) Brad Fitzpatrick is away for a while, and we really want his input > before this CL lands. > 2) The CL as it stands, although it doesn't match what we want package docs > to look like, contains excellent information. > > Therefore we propose: > > 1) Put this info on the wiki. > 2) For now, just put a link to the wiki in the docs with a small update. > > Then when Brad returns we can figure out what we really want. We don't need > to sort this out for the 1.2 timeline. > > Sound OK? > > -rob Sounds good. I'll update the wiki and keep an eye on the issue after 1.2. It's probably worth fixing 5110 at the same time.
Sign in to reply to this message.
On 2013/10/01 23:34:18, r wrote: > In any case, making examples is always welcome. > > -rob I'll a couple of the examples to example_test.go as part of this CL.
Sign in to reply to this message.
On 2013/10/02 10:46:17, mattcottingham wrote: > On 2013/10/01 23:34:18, r wrote: > > In any case, making examples is always welcome. > > > > -rob > > I'll a couple of the examples to example_test.go as part of this CL. And please add a link to the wiki page.
Sign in to reply to this message.
On 2013/10/02 16:42:39, rsc wrote: > On 2013/10/02 10:46:17, mattcottingham wrote: > > On 2013/10/01 23:34:18, r wrote: > > > In any case, making examples is always welcome. > > > > > > -rob > > > > I'll a couple of the examples to example_test.go as part of this CL. > > And please add a link to the wiki page. Could someone add me to the wiki project please?
Sign in to reply to this message.
On Thu, Oct 3, 2013 at 10:16 AM, <MattCottingham@gmail.com> wrote: > Could someone add me to the wiki project please? > Done, I think.
Sign in to reply to this message.
On 2013/10/03 15:00:27, rsc wrote: > On Thu, Oct 3, 2013 at 10:16 AM, <mailto:MattCottingham@gmail.com> wrote: > > > Could someone add me to the wiki project please? > > > > Done, I think. @rsc, I don't know what you did, but it wasn't the right thing to give Matt commit on the wiki, so you might want to undo it @matt, i've added you as a committer on the people tab of the wiki screen (you probably can't see this tab, but trust me)
Sign in to reply to this message.
On 4 October 2013 11:08, <dave@cheney.net> wrote: > @rsc, I don't know what you did, but it wasn't the right thing to give > Matt commit on the wiki, so you might want to undo it > Russ added Matt as a contributor to the go project (wrong). I have undone this. Andrew
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, kamil.kisiel@gmail.com, adg@golang.org, r@golang.org, rsc@golang.org, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
On 2013/10/01 23:34:01, r wrote: > We had a chat about this CL today. Here's what we think: > > 1) Brad Fitzpatrick is away for a while, and we really want his input > before this CL lands. > 2) The CL as it stands, although it doesn't match what we want package docs > to look like, contains excellent information. > > Therefore we propose: > > 1) Put this info on the wiki. > 2) For now, just put a link to the wiki in the docs with a small update. > > Then when Brad returns we can figure out what we really want. We don't need > to sort this out for the 1.2 timeline. > > Sound OK? > > -rob I've added a page to the wiki (https://code.google.com/p/go-wiki/wiki/SQLInterface) and removed doc.go from the CL. Examples for Exec, Begin and Prepare have been added to example_test.go. PTAL
Sign in to reply to this message.
the package comment looks good. can someone more involved with SQL please review the examples?
Sign in to reply to this message.
While we're waiting, please sign the CLA.
Sign in to reply to this message.
It would probably be best to collaborate with https://github.com/VividCortex/go-database-sql-tutorial/blob/master/README.md It's written by Baron Schwartz, one of the authors of the MySQL Performance book by O'Reilly. Sorry if I'm too late to the party to avoid duplication of efforts in writing this...
Sign in to reply to this message.
On 2013/10/08 18:17:51, ahormann wrote: > It would probably be best to collaborate with > https://github.com/VividCortex/go-database-sql-tutorial/blob/master/README.md > It's written by Baron Schwartz, one of the authors of the MySQL Performance > book by O'Reilly. > Sorry if I'm too late to the party to avoid duplication of efforts in > writing this... It's a good document and worth linking to from the wiki page. But I don't think it impacts this CL since the package documentation has only been modified slightly.
Sign in to reply to this message.
On 2013/10/08 16:25:52, r wrote: > While we're waiting, please sign the CLA. Signed.
Sign in to reply to this message.
Will this need to be included in 1.2? I can remove the examples if they're more suited for after this release. Matt On 9 October 2013 17:48, <MattCottingham@gmail.com> wrote: > On 2013/10/08 16:25:52, r wrote: > >> While we're waiting, please sign the CLA. >> > > Signed. > > https://codereview.appspot.**com/14087043/<https://codereview.appspot.com/140... >
Sign in to reply to this message.
https://codereview.appspot.com/14087043/diff/73001/src/pkg/database/sql/examp... File src/pkg/database/sql/example_test.go (right): https://codereview.appspot.com/14087043/diff/73001/src/pkg/database/sql/examp... src/pkg/database/sql/example_test.go:54: id, err := result.LastInsertId() I would probably move LastInsertId to its own example, as a number of databases don't support it and it will just be distracting to some users. https://codereview.appspot.com/14087043/diff/73001/src/pkg/database/sql/examp... src/pkg/database/sql/example_test.go:63: stmt, err := db.Prepare("SELECT name FROM users WHERE age=?") this example doesn't demonstrate that you should prepare once and then call the stmt many times from lots of goroutines. if this were moved into its own file, then it could be a file-level example, with multiple functions, and you could show an init function setting up global variables (with the stmts) and then an http handler using it.
Sign in to reply to this message.
If you'd like to just get in the link at the top of the package docs for now, and work on the examples separately, feel free to send another CL with just the link. Also, you can use the shorter http://golang.org/s/sqlwiki instead if you'd like. On Wed, Oct 16, 2013 at 12:05 AM, Matt Cottingham <mattcottingham@gmail.com>wrote: > Will this need to be included in 1.2? I can remove the examples if they're > more suited for after this release. > > Matt > > > On 9 October 2013 17:48, <MattCottingham@gmail.com> wrote: > >> On 2013/10/08 16:25:52, r wrote: >> >>> While we're waiting, please sign the CLA. >>> >> >> Signed. >> >> https://codereview.appspot.**com/14087043/<https://codereview.appspot.com/140... >> > > -- > > --- > You received this message because you are subscribed to the Google Groups > "golang-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-dev+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out. >
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, kamil.kisiel@gmail.com, adg@golang.org, r@golang.org, rsc@golang.org, dave@cheney.net, arnehormann@gmail.com, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
On 2013/10/16 16:30:43, bradfitz wrote: > If you'd like to just get in the link at the top of the package docs for > now, and work on the examples separately, feel free to send another CL with > just the link. > > Also, you can use the shorter http://golang.org/s/sqlwiki instead if you'd > like. > Sounds good. I've updated the link and reverted example_test.go. I'll open another CL incorporating your review of the examples (probably after 1.2 since I don't want to take up review time so close to a release). Thanks.
Sign in to reply to this message.
On 2013/10/16 16:30:43, bradfitz wrote: > If you'd like to just get in the link at the top of the package docs for > now, and work on the examples separately, feel free to send another CL with > just the link. > > Also, you can use the shorter http://golang.org/s/sqlwiki instead if you'd > like. > Just realised I misread your comment. This CL comprises only the link and I'll send another CL with executable examples. Let me know if I should change it.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=7ebbddd21330 *** database/sql: link to wiki in package docs Update issue 5886 R=golang-dev, kamil.kisiel, adg, r, rsc, dave, arnehormann, bradfitz CC=golang-dev https://codereview.appspot.com/14087043 Committer: Brad Fitzpatrick <bradfitz@golang.org>
Sign in to reply to this message.
|
