Issue 44920043: code review 44920043: database/sql: add more executable examples
|
Created:
10 years, 4 months ago by mattcottingham Modified:
9 years, 4 months ago Reviewers:
CC:
golang-codereviews, gobot, bradfitz Visibility:
Public. |
Description database/sql: add more executable examples
This follows on and incorporates the comments from CL 14087043.
Examples for Begin, Exec and LastInsertID have been added.
An extended example showing a prepared statement being used in an HTTP service has been added.
Patch Set 1 #Patch Set 2 : diff -r 9b9971d5992a https://code.google.com/p/go #Patch Set 3 : diff -r c4b7c0824984 https://code.google.com/p/go #
Total comments: 15
Patch Set 4 : diff -r 459f7bc40d85 https://code.google.com/p/go #Patch Set 5 : diff -r 459f7bc40d85 https://code.google.com/p/go #
Total comments: 2
Patch Set 6 : diff -r 4b3cdcb02f2d https://code.google.com/p/go/ #Patch Set 7 : diff -r 4b3cdcb02f2d https://code.google.com/p/go/ #
MessagesTotal messages: 18
Hello golang-codereviews@googlegroups.com (cc: bradfitz@golang.org), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
R=bradfitz@golang.org (assigned by r@golang.org)
Sign in to reply to this message.
https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... File src/pkg/database/sql/example_prepare_test.go (right): https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... src/pkg/database/sql/example_prepare_test.go:28: func UsersHandler(w http.ResponseWriter, req *http.Request) { I don't think we need an HTTP handler and argument parsing and validation as part of a database example https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... src/pkg/database/sql/example_prepare_test.go:55: func Example_prepare() { there's no prepare in here https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... File src/pkg/database/sql/example_test.go (right): https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... src/pkg/database/sql/example_test.go:49: age := 27 I think the Gopher is more like 5
Sign in to reply to this message.
https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... File src/pkg/database/sql/example_prepare_test.go (right): https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... src/pkg/database/sql/example_prepare_test.go:28: func UsersHandler(w http.ResponseWriter, req *http.Request) { On 2013/12/26 19:01:24, bradfitz wrote: > I don't think we need an HTTP handler and argument parsing and validation as > part of a database example This was based on CL 14087043 where you mentioned an http example. Should I keep the handler? The validation can be removed. https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... src/pkg/database/sql/example_prepare_test.go:55: func Example_prepare() { On 2013/12/26 19:01:24, bradfitz wrote: > there's no prepare in here If it's called main it doesn't appear as an example in the docs (suggestions for another way of doing it appreciated). Though I could split out the setup code in init to give it a name if that's better.
Sign in to reply to this message.
https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... File src/pkg/database/sql/example_prepare_test.go (right): https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... src/pkg/database/sql/example_prepare_test.go:15: // Prepared statement is compiled once and used by multiple goroutines. Go comment style is complete sentences. // userProfileSystem is initialized once and used by multiple goroutines. https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... src/pkg/database/sql/example_prepare_test.go:19: // In real usage, db would be initialized by Open. what is Open? https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... src/pkg/database/sql/example_prepare_test.go:28: func UsersHandler(w http.ResponseWriter, req *http.Request) { On 2013/12/27 12:44:47, mattcottingham wrote: > On 2013/12/26 19:01:24, bradfitz wrote: > > I don't think we need an HTTP handler and argument parsing and validation as > > part of a database example > > This was based on CL 14087043 where you mentioned an http example. Should I keep > the handler? The validation can be removed. Sure, this works. Let's clean up the code a bit, though. https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... src/pkg/database/sql/example_prepare_test.go:33: if len(req.URL.Path) <= len("/users/") { path := req.URL.Path if !strings.HasPrefix(path, "/users/") { ... https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... src/pkg/database/sql/example_prepare_test.go:39: username := req.URL.Path[len("/users/"):] username := strings.TrimPrefix(path, "/users/")
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, gobot@golang.org, bradfitz@golang.org (cc: bradfitz@golang.org, golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
Might be good to trim the handler more (the HTTP method check probably doesn't need to be there). https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... File src/pkg/database/sql/example_prepare_test.go (right): https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... src/pkg/database/sql/example_prepare_test.go:15: // Prepared statement is compiled once and used by multiple goroutines. On 2014/01/14 23:18:58, bradfitz wrote: > Go comment style is complete sentences. > > // userProfileSystem is initialized once and used by multiple goroutines. Done. https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... src/pkg/database/sql/example_prepare_test.go:19: // In real usage, db would be initialized by Open. On 2014/01/14 23:18:58, bradfitz wrote: > what is Open? Done. https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... src/pkg/database/sql/example_prepare_test.go:33: if len(req.URL.Path) <= len("/users/") { On 2014/01/14 23:18:58, bradfitz wrote: > path := req.URL.Path > if !strings.HasPrefix(path, "/users/") { > ... Done. https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... src/pkg/database/sql/example_prepare_test.go:39: username := req.URL.Path[len("/users/"):] On 2014/01/14 23:18:58, bradfitz wrote: > username := strings.TrimPrefix(path, "/users/") Done. https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... File src/pkg/database/sql/example_test.go (right): https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... src/pkg/database/sql/example_test.go:49: age := 27 On 2013/12/26 19:01:24, bradfitz wrote: > I think the Gopher is more like 5 Done :)
Sign in to reply to this message.
On 2014/02/03 20:58:09, mattcottingham wrote: > Might be good to trim the handler more (the HTTP method check probably doesn't > need to be there). > > https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... > File src/pkg/database/sql/example_prepare_test.go (right): > > https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... > src/pkg/database/sql/example_prepare_test.go:15: // Prepared statement is > compiled once and used by multiple goroutines. > On 2014/01/14 23:18:58, bradfitz wrote: > > Go comment style is complete sentences. > > > > // userProfileSystem is initialized once and used by multiple goroutines. > > Done. > > https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... > src/pkg/database/sql/example_prepare_test.go:19: // In real usage, db would be > initialized by Open. > On 2014/01/14 23:18:58, bradfitz wrote: > > what is Open? > > Done. > > https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... > src/pkg/database/sql/example_prepare_test.go:33: if len(req.URL.Path) <= > len("/users/") { > On 2014/01/14 23:18:58, bradfitz wrote: > > path := req.URL.Path > > if !strings.HasPrefix(path, "/users/") { > > ... > > Done. > > https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... > src/pkg/database/sql/example_prepare_test.go:39: username := > req.URL.Path[len("/users/"):] > On 2014/01/14 23:18:58, bradfitz wrote: > > username := strings.TrimPrefix(path, "/users/") > > Done. > > https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... > File src/pkg/database/sql/example_test.go (right): > > https://codereview.appspot.com/44920043/diff/40001/src/pkg/database/sql/examp... > src/pkg/database/sql/example_test.go:49: age := 27 > On 2013/12/26 19:01:24, bradfitz wrote: > > I think the Gopher is more like 5 > > Done :) Ping.
Sign in to reply to this message.
R=bradfitz@golang.org (assigned by MattCottingham@gmail.com)
Sign in to reply to this message.
R=bradfitz@golang.org (assigned by MattCottingham@gmail.com)
Sign in to reply to this message.
Whoops, that was just me clicking the edit button because it was there. I'm not sure what the privileges are supposed to be, but I seem to be able to edit the reviewer through the dashboard at https://go-dev.appspot.com/#all even on CLs that aren't mine. 2014-04-15 13:15 GMT+01:00 <gobot@golang.org>: > R=bradfitz@golang.org (assigned by MattCottingham@gmail.com) > > https://codereview.appspot.com/44920043/ >
Sign in to reply to this message.
https://codereview.appspot.com/44920043/diff/80001/src/pkg/database/sql/examp... File src/pkg/database/sql/example_prepare_test.go (right): https://codereview.appspot.com/44920043/diff/80001/src/pkg/database/sql/examp... src/pkg/database/sql/example_prepare_test.go:57: func Example_prepare() { why is prepare lowercase here? does it have to be? There's also no prepare inside it. https://codereview.appspot.com/44920043/diff/80001/src/pkg/database/sql/examp... File src/pkg/database/sql/example_test.go (right): https://codereview.appspot.com/44920043/diff/80001/src/pkg/database/sql/examp... src/pkg/database/sql/example_test.go:57: func Result_LastInsertId() { is this missing the Example prefix?
Sign in to reply to this message.
On 2014/05/13 21:37:43, bradfitz wrote: > https://codereview.appspot.com/44920043/diff/80001/src/pkg/database/sql/examp... > File src/pkg/database/sql/example_prepare_test.go (right): > > https://codereview.appspot.com/44920043/diff/80001/src/pkg/database/sql/examp... > src/pkg/database/sql/example_prepare_test.go:57: func Example_prepare() { > why is prepare lowercase here? does it have to be? There's also no prepare > inside it. I think this can just be called Example() for a package example. I've changed it to that. I've renamed the file to reflect that it's demonstrating concurrent use of a prepared stmt. > > https://codereview.appspot.com/44920043/diff/80001/src/pkg/database/sql/examp... > File src/pkg/database/sql/example_test.go (right): > > https://codereview.appspot.com/44920043/diff/80001/src/pkg/database/sql/examp... > src/pkg/database/sql/example_test.go:57: func Result_LastInsertId() { > is this missing the Example prefix? LastInsertID doesn't have its own listing in the doc so I've removed this.
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, gobot@golang.org, bradfitz@golang.org (cc: bradfitz@golang.org, golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
On 2014/05/26 20:08:11, mattcottingham wrote: > Hello mailto:golang-codereviews@googlegroups.com, mailto:gobot@golang.org, mailto:bradfitz@golang.org > (cc: mailto:bradfitz@golang.org, mailto:golang-codereviews@googlegroups.com), > > Please take another look. Has this missed the boat for the next release? go-dev.appspot.com no longer shows the 1.3 tag and issue 5886 was closed with another CL.
Sign in to reply to this message.
The 1.3 ship sailed a long time ago. On Mon, Jun 16, 2014 at 12:18 PM, <MattCottingham@gmail.com> wrote: > On 2014/05/26 20:08:11, mattcottingham wrote: > >> Hello mailto:golang-codereviews@googlegroups.com, >> > mailto:gobot@golang.org, mailto:bradfitz@golang.org > >> (cc: mailto:bradfitz@golang.org, >> > mailto:golang-codereviews@googlegroups.com), > > Please take another look. >> > > Has this missed the boat for the next release? go-dev.appspot.com no > longer shows the 1.3 tag and issue 5886 was closed with another CL. > > https://codereview.appspot.com/44920043/ >
Sign in to reply to this message.
Ping. On 2014/06/16 19:21:38, bradfitz wrote: > The 1.3 ship sailed a long time ago. >
Sign in to reply to this message.
R=close To the author of this CL: The Go project has moved to Gerrit Code Review. If this CL should be continued, please see the latest version of https://golang.org/doc/contribute.html for instructions on how to set up Git and the Go project's Gerrit codereview plugin, and then create a new change with your current code. If there has been discussion on this CL, please give a link to it (golang.org/cl/44920043 is best) in the description in your new CL. Thanks very much.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
