|
|
Descriptionmisc: add goplay
Patch Set 1 #Patch Set 2 : code review 2473041: misc: add goplay #
Total comments: 4
Patch Set 3 : code review 2473041: misc: add goplay #Patch Set 4 : code review 2473041: misc: add goplay #
Total comments: 26
Patch Set 5 : code review 2473041: misc: add goplay #
Total comments: 4
Patch Set 6 : code review 2473041: misc: add goplay #
Total comments: 4
Patch Set 7 : code review 2473041: misc: add goplay #
Total comments: 4
Patch Set 8 : code review 2473041: misc: add goplay #Patch Set 9 : code review 2473041: misc: add goplay #
MessagesTotal messages: 17
Hello rsc (cc: golang-dev@googlegroups.com), I'd like you to review this change.
Sign in to reply to this message.
Should probably have a short README.
Sign in to reply to this message.
Hello rsc (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... misc/goplay/goplay.go:169: var frontPageText = `<!doctype html> have to trust you on this one
Sign in to reply to this message.
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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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>
Sign in to reply to this message.
Hello rsc, r (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
Hello rsc, r (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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#newcod... misc/goplay/goplay.go:173: var outputText = "<pre>{@|html}</pre>" i'd use `` here for consistency
Sign in to reply to this message.
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#newcod... 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.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** 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
Sign in to reply to this message.
|