Code review - Issue 2473041: code review 2473041: misc: add goplayhttps://codereview.appspot.com/2010-10-14T03:06:07+00:00rietveld
Message from unknown
2010-10-13T04:13:05+00:00adgurn:md5:181824ce0912eaf8ef35c83410b91988
Message from adg@golang.org
2010-10-13T04:13:13+00:00adgurn:md5:905e8642e3228b9750799e4e9416e2bb
Hello rsc (cc: golang-dev@googlegroups.com),
I'd like you to review this change.
Message from rsc@golang.org
2010-10-13T04:39:35+00:00rscurn:md5:8c1dc42f9c080cb8c3a3a64fd8022c4e
Should probably have a short README.
Message from unknown
2010-10-13T05:02:32+00:00adgurn:md5:4a2f1b5d906919e98bd5289f9e177d83
Message from adg@golang.org
2010-10-13T05:02:38+00:00adgurn:md5:70ac06524c39e1c9e6ac043f2217ed33
Hello rsc (cc: golang-dev@googlegroups.com),
Please take another look.
Message from r@golang.org
2010-10-13T05:09:35+00:00rurn:md5:bcad823f808b8c358635cddcb4cb76e6
http://codereview.appspot.com/2473041/diff/4001/misc/goplay/README
File misc/goplay/README (right):
http://codereview.appspot.com/2473041/diff/4001/misc/goplay/README#newcode9
misc/goplay/README:9: and load http://localhost:3999/ in a web browser. Chrome and Firefox work well.
s/ Chrome.*//
http://codereview.appspot.com/2473041/diff/4001/misc/goplay/goplay.go
File misc/goplay/goplay.go (right):
http://codereview.appspot.com/2473041/diff/4001/misc/goplay/goplay.go#newcode5
misc/goplay/goplay.go:5: package main
there is not a single comment in this code.
Message from unknown
2010-10-13T05:33:59+00:00adgurn:md5:5dcbe5fcda595880ea80db63e24c1826
Message from adg@golang.org
2010-10-13T05:34:08+00:00adgurn:md5:962ea2b2d98e4852ef063a3f56f5cda3
PTAL
http://codereview.appspot.com/2473041/diff/4001/misc/goplay/README
File misc/goplay/README (right):
http://codereview.appspot.com/2473041/diff/4001/misc/goplay/README#newcode9
misc/goplay/README:9: and load http://localhost:3999/ in a web browser. Chrome and Firefox work well.
On 2010/10/13 05:09:35, r wrote:
> s/ Chrome.*//
Done.
http://codereview.appspot.com/2473041/diff/4001/misc/goplay/goplay.go
File misc/goplay/goplay.go (right):
http://codereview.appspot.com/2473041/diff/4001/misc/goplay/goplay.go#newcode5
misc/goplay/goplay.go:5: package main
On 2010/10/13 05:09:35, r wrote:
> there is not a single comment in this code.
I've now commented it.
Message from unknown
2010-10-13T05:35:37+00:00adgurn:md5:4668db701f13b496527db9d61832f820
Message from r@golang.org
2010-10-13T05:50:20+00:00rurn:md5:4f59c82653b2b1b0486bb36e1a37c256
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/README
File misc/goplay/README (right):
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/README#newcode1
misc/goplay/README:1: Goplay is a web interface for experimenting with Go code.
this file should have a copyright (they all do) and be called doc.go and look like go code. see the other commands for an example.
the README could just point to doc.go
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go
File misc/goplay/goplay.go (right):
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go#newcode28
misc/goplay/goplay.go:28: // the architecture of the toolchain (ie, 6g/8g)
toolchain is not a word. this is english, not german
latin abbreviations need periods and anyway you mean e.g. but you don't mean 6g/8g, you mean 5, 6, or 8
// the architecture-identifying character of the tool chain, typically 5, 6, or 8
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go#newcode29
misc/goplay/goplay.go:29: toolchar = "6"
s;$; // amd64 by default;
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go#newcode29
misc/goplay/goplay.go:29: toolchar = "6"
if toolchar was actually ".6" some code would get nicer.
maybe you should have "objSuffix" and call it ".6"
anyway s/toolchar/archChar/
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go#newcode41
misc/goplay/goplay.go:41: toolchar = "8"
what about arm? it's starting to work; might as well be prepared.
don't we have a library to help us with this?
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go#newcode58
misc/goplay/goplay.go:58: // its contents will be put in the interface's textarea.
s/textarea/text area/ mein Herr.
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go#newcode59
misc/goplay/goplay.go:59: // Otherwise, the default Hello World program is displayed.
s/Hello World/"hello, world"/
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go#newcode76
misc/goplay/goplay.go:76: f, err := os.Open(x+".go", os.O_CREAT|os.O_WRONLY, 0666)
O_TRUNC should be there too
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go#newcode113
misc/goplay/goplay.go:113: w.Write([]byte("<pre style=\"font-size: 100%;\">"))
ResponseWriter could sure use a WriteString method
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go#newcode115
misc/goplay/goplay.go:115: w.Write([]byte("</pre>"))
it's up to you but pedantic me would make these variables public so we wouldn't need to convert them every time, although the cost is negligible.
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go#newcode131
misc/goplay/goplay.go:131: // run executes the specified command, and returns its output and an error.
s/,//
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go#newcode140
misc/goplay/goplay.go:140: // run the binary, and read its combined stdout and stderr into a buffer
s/,//
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go#newcode169
misc/goplay/goplay.go:169: var frontPageText = `<!doctype html>
have to trust you on this one
Message from unknown
2010-10-13T06:10:44+00:00adgurn:md5:b9d0bd37449c25742f57892bc30cbb7f
Message from adg@golang.org
2010-10-13T06:10:49+00:00adgurn:md5:3a2d9c92fbc0f626642ab5200ecac884
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/README
File misc/goplay/README (right):
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/README#newcode1
misc/goplay/README:1: Goplay is a web interface for experimenting with Go code.
On 2010/10/13 05:50:20, r wrote:
> this file should have a copyright (they all do) and be called doc.go and look
> like go code. see the other commands for an example.
>
> the README could just point to doc.go
Done.
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go
File misc/goplay/goplay.go (right):
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go#newcode28
misc/goplay/goplay.go:28: // the architecture of the toolchain (ie, 6g/8g)
On 2010/10/13 05:50:20, r wrote:
> toolchain is not a word. this is english, not german
>
> latin abbreviations need periods and anyway you mean e.g. but you don't mean
> 6g/8g, you mean 5, 6, or 8
>
> // the architecture-identifying character of the tool chain, typically 5, 6, or
> 8
Done.
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go#newcode29
misc/goplay/goplay.go:29: toolchar = "6"
On 2010/10/13 05:50:20, r wrote:
> s;$; // amd64 by default;
Done.
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go#newcode29
misc/goplay/goplay.go:29: toolchar = "6"
On 2010/10/13 05:50:20, r wrote:
> if toolchar was actually ".6" some code would get nicer.
> maybe you should have "objSuffix" and call it ".6"
> anyway s/toolchar/archChar/
Done.
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go#newcode41
misc/goplay/goplay.go:41: toolchar = "8"
On 2010/10/13 05:50:20, r wrote:
> what about arm? it's starting to work; might as well be prepared.
Done.
> don't we have a library to help us with this?
Not really.
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go#newcode58
misc/goplay/goplay.go:58: // its contents will be put in the interface's textarea.
On 2010/10/13 05:50:20, r wrote:
> s/textarea/text area/ mein Herr.
It actually is an HTML "TEXTAREA" but done.
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go#newcode59
misc/goplay/goplay.go:59: // Otherwise, the default Hello World program is displayed.
On 2010/10/13 05:50:20, r wrote:
> s/Hello World/"hello, world"/
Done.
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go#newcode76
misc/goplay/goplay.go:76: f, err := os.Open(x+".go", os.O_CREAT|os.O_WRONLY, 0666)
On 2010/10/13 05:50:20, r wrote:
> O_TRUNC should be there too
Done.
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go#newcode113
misc/goplay/goplay.go:113: w.Write([]byte("<pre style=\"font-size: 100%;\">"))
On 2010/10/13 05:50:20, r wrote:
> ResponseWriter could sure use a WriteString method
Maybe. I switched to using a template instead.
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go#newcode115
misc/goplay/goplay.go:115: w.Write([]byte("</pre>"))
On 2010/10/13 05:50:20, r wrote:
> it's up to you but pedantic me would make these variables public so we wouldn't
> need to convert them every time, although the cost is negligible.
Done.
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go#newcode131
misc/goplay/goplay.go:131: // run executes the specified command, and returns its output and an error.
On 2010/10/13 05:50:20, r wrote:
> s/,//
Done.
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go#newcode140
misc/goplay/goplay.go:140: // run the binary, and read its combined stdout and stderr into a buffer
On 2010/10/13 05:50:20, r wrote:
> s/,//
Done.
http://codereview.appspot.com/2473041/diff/13001/misc/goplay/goplay.go#newcode169
misc/goplay/goplay.go:169: var frontPageText = `<!doctype html>
On 2010/10/13 05:50:20, r wrote:
> have to trust you on this one
This is actually what HTML5 looks like.
At least it's nicer than <!doctype XHTML1.0 DOCSTRING HOCUS POCUS FOOBAR>
Message from adg@golang.org
2010-10-13T06:10:50+00:00adgurn:md5:44ff4dcde4830808e42b5dce608c1bd0
Hello rsc, r (cc: golang-dev@googlegroups.com),
Please take another look.
Message from rsc@google.com
2010-10-13T16:43:59+00:00rsc1urn:md5:4250fe8e519f5a97831606ba8f39c9cc
http://codereview.appspot.com/2473041/diff/11005/misc/goplay/goplay.go
File misc/goplay/goplay.go (right):
http://codereview.appspot.com/2473041/diff/11005/misc/goplay/goplay.go#newcode22
misc/goplay/goplay.go:22: goarch = flag.String("goarch", "", "target architecture (defaults to environment)")
delete (see below)
http://codereview.appspot.com/2473041/diff/11005/misc/goplay/goplay.go#newcode30
misc/goplay/goplay.go:30: objSuffix = ".6"
this was a clumsy hack that predated not setting goarch.
i think you want to just declare string here with no init,
and then below check runtime.GOARCH.
don't bother with os.Getenv
Message from unknown
2010-10-13T23:15:46+00:00adgurn:md5:36fa209ab4229aae9305b7310347280e
Message from adg@golang.org
2010-10-13T23:16:00+00:00adgurn:md5:b575bab23d6e1b1944b9b29308689ba1
http://codereview.appspot.com/2473041/diff/11005/misc/goplay/goplay.go
File misc/goplay/goplay.go (right):
http://codereview.appspot.com/2473041/diff/11005/misc/goplay/goplay.go#newcode22
misc/goplay/goplay.go:22: goarch = flag.String("goarch", "", "target architecture (defaults to environment)")
On 2010/10/13 16:43:59, rsc1 wrote:
> delete (see below)
Done.
http://codereview.appspot.com/2473041/diff/11005/misc/goplay/goplay.go#newcode30
misc/goplay/goplay.go:30: objSuffix = ".6"
On 2010/10/13 16:43:59, rsc1 wrote:
> this was a clumsy hack that predated not setting goarch.
> i think you want to just declare string here with no init,
> and then below check runtime.GOARCH.
> don't bother with os.Getenv
Done.
I don't know why I didn't think of this when Rob suggested I automated it. I blame codeine.
Message from rsc@google.com
2010-10-14T00:15:52+00:00rsc1urn:md5:0878e3e2ea40f5291e9a60cff7ce92e8
http://codereview.appspot.com/2473041/diff/22001/misc/goplay/goplay.go
File misc/goplay/goplay.go (right):
http://codereview.appspot.com/2473041/diff/22001/misc/goplay/goplay.go#newcode30
misc/goplay/goplay.go:30: archChar = "6" // amd64 by default
var archChar, objSuffix string
put "amd64" in the switch below.
and a default case that does
fmt.Fprintf(os.Stderr, "unknown GOARCH: %s\n", runtime.GOARCH)
os.Exit(2)
http://codereview.appspot.com/2473041/diff/22001/misc/goplay/goplay.go#newcode44
misc/goplay/goplay.go:44: objSuffix = "." + archChar
not really sure this variable pulls its weight
one line here saves "."+ twice below
Message from unknown
2010-10-14T00:24:17+00:00adgurn:md5:30492f2affe5a669351b0d10a9a871c8
Message from adg@golang.org
2010-10-14T00:24:23+00:00adgurn:md5:70e8106f602f58633478c9a2e746d737
Hello rsc, r (cc: golang-dev@googlegroups.com),
Please take another look.
Message from adg@golang.org
2010-10-14T00:26:16+00:00adgurn:md5:3eaec4161e1510717814a99ec1046868
http://codereview.appspot.com/2473041/diff/22001/misc/goplay/goplay.go
File misc/goplay/goplay.go (right):
http://codereview.appspot.com/2473041/diff/22001/misc/goplay/goplay.go#newcode30
misc/goplay/goplay.go:30: archChar = "6" // amd64 by default
On 2010/10/14 00:15:52, rsc1 wrote:
> var archChar, objSuffix string
>
> put "amd64" in the switch below.
> and a default case that does
>
> fmt.Fprintf(os.Stderr, "unknown GOARCH: %s\n", runtime.GOARCH)
> os.Exit(2)
>
Done.
http://codereview.appspot.com/2473041/diff/22001/misc/goplay/goplay.go#newcode44
misc/goplay/goplay.go:44: objSuffix = "." + archChar
On 2010/10/14 00:15:52, rsc1 wrote:
> not really sure this variable pulls its weight
> one line here saves "."+ twice below
Done.
Message from r@golang.org
2010-10-14T00:29:06+00:00rurn:md5:e9cfe6f3652fba2150a80d0100354706
LGTM but wait for rsc
http://codereview.appspot.com/2473041/diff/15003/misc/goplay/goplay.go
File misc/goplay/goplay.go (right):
http://codereview.appspot.com/2473041/diff/15003/misc/goplay/goplay.go#newcode45
misc/goplay/goplay.go:45: log.Exit("unrecognized GOARCH:", runtime.GOARCH)
s/:/: / or use Exitln
http://codereview.appspot.com/2473041/diff/15003/misc/goplay/goplay.go#newcode173
misc/goplay/goplay.go:173: var outputText = "<pre>{@|html}</pre>"
i'd use `` here for consistency
Message from unknown
2010-10-14T00:32:42+00:00adgurn:md5:102b2f265d9214aee3f81ac537b614d0
Message from adg@golang.org
2010-10-14T00:34:29+00:00adgurn:md5:95c9ca8cacc03ad13b9aa53ab315e250
http://codereview.appspot.com/2473041/diff/15003/misc/goplay/goplay.go
File misc/goplay/goplay.go (right):
http://codereview.appspot.com/2473041/diff/15003/misc/goplay/goplay.go#newcode45
misc/goplay/goplay.go:45: log.Exit("unrecognized GOARCH:", runtime.GOARCH)
On 2010/10/14 00:29:06, r wrote:
> s/:/: / or use Exitln
Done.
http://codereview.appspot.com/2473041/diff/15003/misc/goplay/goplay.go#newcode173
misc/goplay/goplay.go:173: var outputText = "<pre>{@|html}</pre>"
On 2010/10/14 00:29:06, r wrote:
> i'd use `` here for consistency
Done.
Message from rsc@golang.org
2010-10-14T02:01:19+00:00rscurn:md5:f732eb6a7874d58f12d7730d28ffc78b
LGTM
Message from unknown
2010-10-14T03:05:56+00:00adgurn:md5:8c275ac0af9e7e2b5aa46ee8443ff916
Message from adg@golang.org
2010-10-14T03:06:07+00:00adgurn:md5:32b71264b54b2e3c59181fc26c3b3393
*** Submitted as http://code.google.com/p/go/source/detail?r=980f30fafac2 ***
misc: add goplay
R=rsc, r
CC=golang-dev
http://codereview.appspot.com/2473041