Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
LGTM http://codereview.appspot.com/5869050/diff/1012/src/cmd/godoc/godoc.go File src/cmd/godoc/godoc.go (right): http://codereview.appspot.com/5869050/diff/1012/src/cmd/godoc/godoc.go#newcod... src/cmd/godoc/godoc.go:549: type Page struct { doc comment? http://codereview.appspot.com/5869050/diff/1012/src/cmd/godoc/godoc.go#newcod... src/cmd/godoc/godoc.go:554: Contents []byte Body is a better name than Contents, IMO
PTAL http://codereview.appspot.com/5869050/diff/1012/src/cmd/godoc/godoc.go File src/cmd/godoc/godoc.go (right): http://codereview.appspot.com/5869050/diff/1012/src/cmd/godoc/godoc.go#newcod... src/cmd/godoc/godoc.go:549: type Page struct { On 2012/03/22 05:31:33, adg wrote: > doc comment? Done. http://codereview.appspot.com/5869050/diff/1012/src/cmd/godoc/godoc.go#newcod... src/cmd/godoc/godoc.go:554: Contents []byte On 2012/03/22 05:31:33, adg wrote: > Body is a better name than Contents, IMO Done.
LGTM but why not Page values instead of pointers to them? They're kinda big, but not too big, and not any larger than you were passing in arguments before anyway. On Thu, Mar 22, 2012 at 10:58 AM, <gri@golang.org> wrote: > PTAL > > > > http://codereview.appspot.com/**5869050/diff/1012/src/cmd/**godoc/godoc.go<ht... > File src/cmd/godoc/godoc.go (right): > > http://codereview.appspot.com/**5869050/diff/1012/src/cmd/** > godoc/godoc.go#newcode549<http://codereview.appspot.com/5869050/diff/1012/src/cmd/godoc/godoc.go#newcode549> > src/cmd/godoc/godoc.go:549: type Page struct { > On 2012/03/22 05:31:33, adg wrote: > >> doc comment? >> > > Done. > > > http://codereview.appspot.com/**5869050/diff/1012/src/cmd/** > godoc/godoc.go#newcode554<http://codereview.appspot.com/5869050/diff/1012/src/cmd/godoc/godoc.go#newcode554> > src/cmd/godoc/godoc.go:554: Contents []byte > On 2012/03/22 05:31:33, adg wrote: > >> Body is a better name than Contents, IMO >> > > Done. > > http://codereview.appspot.com/**5869050/<http://codereview.appspot.com/5869050/> >
Done. On Thu, Mar 22, 2012 at 11:02 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > LGTM but why not Page values instead of pointers to them? They're kinda > big, but not too big, and not any larger than you were passing in arguments > before anyway. > > On Thu, Mar 22, 2012 at 10:58 AM, <gri@golang.org> wrote: >> >> PTAL >> >> >> >> http://codereview.appspot.com/5869050/diff/1012/src/cmd/godoc/godoc.go >> File src/cmd/godoc/godoc.go (right): >> >> >> http://codereview.appspot.com/5869050/diff/1012/src/cmd/godoc/godoc.go#newcod... >> src/cmd/godoc/godoc.go:549: type Page struct { >> On 2012/03/22 05:31:33, adg wrote: >>> >>> doc comment? >> >> >> Done. >> >> >> >> http://codereview.appspot.com/5869050/diff/1012/src/cmd/godoc/godoc.go#newcod... >> src/cmd/godoc/godoc.go:554: Contents []byte >> On 2012/03/22 05:31:33, adg wrote: >>> >>> Body is a better name than Contents, IMO >> >> >> Done. >> >> http://codereview.appspot.com/5869050/ > >
LGTM On Thu, Mar 22, 2012 at 11:11 AM, Robert Griesemer <gri@golang.org> wrote: > Done. > > On Thu, Mar 22, 2012 at 11:02 AM, Brad Fitzpatrick <bradfitz@golang.org> > wrote: > > LGTM but why not Page values instead of pointers to them? They're kinda > > big, but not too big, and not any larger than you were passing in > arguments > > before anyway. > > > > On Thu, Mar 22, 2012 at 10:58 AM, <gri@golang.org> wrote: > >> > >> PTAL > >> > >> > >> > >> http://codereview.appspot.com/5869050/diff/1012/src/cmd/godoc/godoc.go > >> File src/cmd/godoc/godoc.go (right): > >> > >> > >> > http://codereview.appspot.com/5869050/diff/1012/src/cmd/godoc/godoc.go#newcod... > >> src/cmd/godoc/godoc.go:549: type Page struct { > >> On 2012/03/22 05:31:33, adg wrote: > >>> > >>> doc comment? > >> > >> > >> Done. > >> > >> > >> > >> > http://codereview.appspot.com/5869050/diff/1012/src/cmd/godoc/godoc.go#newcod... > >> src/cmd/godoc/godoc.go:554: Contents []byte > >> On 2012/03/22 05:31:33, adg wrote: > >>> > >>> Body is a better name than Contents, IMO > >> > >> > >> Done. > >> > >> http://codereview.appspot.com/5869050/ > > > > >
*** Submitted as http://code.google.com/p/go/source/detail?r=74dc15b37455 *** godoc: replace servePage's positional argument list R=golang-dev, adg, bradfitz CC=golang-dev http://codereview.appspot.com/5869050
