code review 14087043: database/sql: Improve package documentation (issue 14087043)

[MattCot...@gmail.com]

Reviewers: golang-dev1,

Message:
Hello golan...@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go


Description:
database/sql: Improve package documentation

Adds implementation-independent documentation
for connecting to and querying databases.

Fixes issue 5886.

Please review this at https://codereview.appspot.com/14087043/

Affected files (+105, -5 lines):
A src/pkg/database/sql/doc.go
M src/pkg/database/sql/sql.go

[n...@nathany.com]

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#newcode18
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...)?

https://codereview.appspot.com/14087043/

[kamil....@gmail.com]

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#newcode36
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#newcode42
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#newcode60
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?

https://codereview.appspot.com/14087043/

[MattCot...@gmail.com]

Hello golan...@googlegroups.com, kamil....@gmail.com (cc:
golan...@googlegroups.com),

Please take another look.


https://codereview.appspot.com/14087043/

[MattCot...@gmail.com]

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#newcode18
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#newcode36
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#newcode42
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#newcode60
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.

https://codereview.appspot.com/14087043/

[kamil....@gmail.com]

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.go#newcode18
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.go#newcode22
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.go#newcode42
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.go#newcode75
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.

https://codereview.appspot.com/14087043/

[MattCot...@gmail.com]

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.go#newcode18
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.go#newcode22
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.go#newcode42
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.go#newcode75
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.

https://codereview.appspot.com/14087043/

[MattCot...@gmail.com]

[kamil....@gmail.com]

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.go#newcode36

src/pkg/database/sql/doc.go:36: result, err:= db.Exec(

missing a space between err and :

https://codereview.appspot.com/14087043/

[MattCot...@gmail.com]

Hello golan...@googlegroups.com, kamil....@gmail.com,
a...@golang.org (cc: golan...@googlegroups.com),

[r...@golang.org]

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.


https://codereview.appspot.com/14087043/

[MattCot...@gmail.com]

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?

https://codereview.appspot.com/14087043/

[kamil....@gmail.com]

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?

https://codereview.appspot.com/14087043/

[Rob Pike]

There's an open bug to fix the documentation for package net. Please don't use it as a style guide.

-rob

[MattCot...@gmail.com]

Thanks for clarifying -- I didn't see the ticket.

https://codereview.appspot.com/14087043/

[MattCot...@gmail.com]

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.

https://codereview.appspot.com/14087043/

[Rob Pike]

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

[Rob Pike]

In any case, making examples is always welcome.

-rob

[MattCot...@gmail.com]

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.

https://codereview.appspot.com/14087043/

[MattCot...@gmail.com]

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.

https://codereview.appspot.com/14087043/

[r...@golang.org]

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.


https://codereview.appspot.com/14087043/

[MattCot...@gmail.com]

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?

https://codereview.appspot.com/14087043/

[Russ Cox]

On Thu, Oct 3, 2013 at 10:16 AM, <MattCot...@gmail.com> wrote:

Could someone add me to the wiki project please?

Done, I think.