code review 1008042: godoc: add codewalk support

godoc: add codewalk support

[rsc, 2010-04-26 08:10:37]

Hello adg (cc: golang-dev@googlegroups.com, gri, r),

I'd like you to review this change.

[adg, 2010-04-26 12:57:09]

Generally looks pretty good, however the codewalk listing (visiting
/doc/codewalk/) doesn't work for me. The following message is printed to the
console, and I get a blank page titled "Codewalks".

2010/04/26 22:55:46 codewalkdir.Execute: line 16: name not found: Name

http://codereview.appspot.com/1008042/diff/7001/8006
File lib/godoc/codewalk.html (right):

http://codereview.appspot.com/1008042/diff/7001/8006#newcode6
lib/godoc/codewalk.html:6: TODO BEFORE REVIEW: HTML escaping
?

http://codereview.appspot.com/1008042/diff/7001/8006#newcode10
lib/godoc/codewalk.html:10: <script type='text/javascript'
src='http://jqueryjs.googlecode.com/files/jquery-1.3.2.min.js'></script>
I tried replacing this with the latest version of jQuery using jsapi, and it
broke codewalk.js. I'll fix it in a follow-up CL.

http://codereview.appspot.com/1008042/diff/7001/8007
File lib/godoc/codewalkdir.html (right):

http://codereview.appspot.com/1008042/diff/7001/8007#newcode11
lib/godoc/codewalkdir.html:11: <td width=25">&nbsp;</td>
s/25"/"25%"/

[rsc, 2010-04-26 16:51:50]

On Mon, Apr 26, 2010 at 05:57,  <adg@golang.org> wrote:
> Generally looks pretty good, however the codewalk listing (visiting
> /doc/codewalk/) doesn't work for me. The following message is printed to
> the console, and I get a blank page titled "Codewalks".

I meant to mention in the CL desc that in order to run this
you also need CLs 1008041, 1009041, and 982043, which
fix various tiny bugs I ran into in other packages.

Russ

[rsc1, 2010-04-26 16:58:44]

PTAL

http://codereview.appspot.com/1008042/diff/7001/8006
File lib/godoc/codewalk.html (right):

http://codereview.appspot.com/1008042/diff/7001/8006#newcode6
lib/godoc/codewalk.html:6: TODO BEFORE REVIEW: HTML escaping
On 2010/04/26 12:57:10, adg wrote:
> ?

Did this and forgot to write out the file.

http://codereview.appspot.com/1008042/diff/7001/8006#newcode10
lib/godoc/codewalk.html:10: <script type='text/javascript'
src='http://jqueryjs.googlecode.com/files/jquery-1.3.2.min.js'></script>
On 2010/04/26 12:57:10, adg wrote:
> I tried replacing this with the latest version of jQuery using jsapi, and it
> broke codewalk.js. I'll fix it in a follow-up CL.

Thanks.

http://codereview.appspot.com/1008042/diff/7001/8007
File lib/godoc/codewalkdir.html (right):

http://codereview.appspot.com/1008042/diff/7001/8007#newcode11
lib/godoc/codewalkdir.html:11: <td width=25">&nbsp;</td>
On 2010/04/26 12:57:10, adg wrote:
> s/25"/"25%"/

Actually "25".  It's a spacer.  Thanks.

[rsc, 2010-04-26 17:24:53]

On Mon, Apr 26, 2010 at 09:51, Russ Cox <rsc@golang.org> wrote:
> On Mon, Apr 26, 2010 at 05:57,  <adg@golang.org> wrote:
>> Generally looks pretty good, however the codewalk listing (visiting
>> /doc/codewalk/) doesn't work for me. The following message is printed to
>> the console, and I get a blank page titled "Codewalks".
>
> I meant to mention in the CL desc that in order to run this
> you also need CLs 1008041, 1009041, and 982043, which
> fix various tiny bugs I ran into in other packages.

Everything codewalk depends on should now be checked in.

Russ

[gri, 2010-04-26 18:24:58]

FYI

http://codereview.appspot.com/1008042/diff/15001/16009
File src/cmd/godoc/codewalk.go (right):

http://codereview.appspot.com/1008042/diff/15001/16009#newcode105
src/cmd/godoc/codewalk.go:105: 
FYI: In all other godoc files inter-function spacing is 2 empty lines

http://codereview.appspot.com/1008042/diff/15001/16009#newcode233
src/cmd/godoc/codewalk.go:233: io.WriteString(c, `<style type="text/css">@import
"/doc/codewalk/codewalk.css";</style><pre>`)
Perhaps move this hardwired address into a const (at the top of the file) so
it's easier to find should it ever change?

http://codereview.appspot.com/1008042/diff/15001/16009#newcode250
src/cmd/godoc/codewalk.go:250: func addrToByte(addr string, start int, data
[]byte) (lo, hi int, err os.Error) {
addrToRange? It's a range that's returned, not a single byte

http://codereview.appspot.com/1008042/diff/15001/16009#newcode341
src/cmd/godoc/codewalk.go:341: // dir is '+' or '-', n is the count, and
charOffset is true if the syntax used was #n.
What does "apply" mean in this context. What is the result?

http://codereview.appspot.com/1008042/diff/15001/16009#newcode423
src/cmd/godoc/codewalk.go:423: // but it is not implemented.
ditto

http://codereview.appspot.com/1008042/diff/15001/16009#newcode457
src/cmd/godoc/codewalk.go:457: if n--; n == 0 {
move the n-- outside the if. as it is, it almost looks like as if it were
executed conditionally. at the very least, it's confusing

[adg, 2010-04-27 04:57:18]

LGTM

Works really well now.

On 27 April 2010 03:24, Russ Cox <rsc@golang.org> wrote:
> Everything codewalk depends on should now be checked in.

[adg, 2010-04-27 05:03:53]

JSAPI stuff below. It works if you force the version. We can update to the
latest jQuery later as discussed.

http://codereview.appspot.com/1008042/diff/15001/16006
File lib/godoc/codewalk.html (right):

http://codereview.appspot.com/1008042/diff/15001/16006#newcode7
lib/godoc/codewalk.html:7: <script type='text/javascript'
src='http://jqueryjs.googlecode.com/files/jquery-1.3.2.min.js'></script>
You can safely replace the above with:

<script src="http://www.google.com/jsapi"></script>
<script>
  google.load("jquery", "1.3.2");
</script>

[rsc1, 2010-04-27 05:34:13]

Thanks for the reviews.

http://codereview.appspot.com/1008042/diff/15001/16006
File lib/godoc/codewalk.html (right):

http://codereview.appspot.com/1008042/diff/15001/16006#newcode7
lib/godoc/codewalk.html:7: <script type='text/javascript'
src='http://jqueryjs.googlecode.com/files/jquery-1.3.2.min.js'></script>
On 2010/04/27 05:03:53, adg wrote:
> You can safely replace the above with:
> 
> <script src="http://www.google.com/jsapi"></script>
> <script>
>   google.load("jquery", "1.3.2");
> </script>
> 

Done.

http://codereview.appspot.com/1008042/diff/15001/16009
File src/cmd/godoc/codewalk.go (right):

http://codereview.appspot.com/1008042/diff/15001/16009#newcode105
src/cmd/godoc/codewalk.go:105: 
On 2010/04/26 18:24:58, gri wrote:
> FYI: In all other godoc files inter-function spacing is 2 empty lines

Lots of blank lines added, though I wish we could pick one style
and let gofmt do the job.

http://codereview.appspot.com/1008042/diff/15001/16009#newcode233
src/cmd/godoc/codewalk.go:233: io.WriteString(c, `<style type="text/css">@import
"/doc/codewalk/codewalk.css";</style><pre>`)
On 2010/04/26 18:24:58, gri wrote:
> Perhaps move this hardwired address into a const (at the top of the file) so
> it's easier to find should it ever change?

It's right on the line but I left it.  I could define 5 constants
somewhere else but then they're somewhere else.
I thought about using a template for the whole block but
the logic (specifically the lo < hi) is just a little too complex.

http://codereview.appspot.com/1008042/diff/15001/16009#newcode250
src/cmd/godoc/codewalk.go:250: func addrToByte(addr string, start int, data
[]byte) (lo, hi int, err os.Error) {
On 2010/04/26 18:24:58, gri wrote:
> addrToRange? It's a range that's returned, not a single byte

It's important that the result is a byte offset,
because there are other functions that deal
in line offsets.  Changed to addrToByteRange.

http://codereview.appspot.com/1008042/diff/15001/16009#newcode341
src/cmd/godoc/codewalk.go:341: // dir is '+' or '-', n is the count, and
charOffset is true if the syntax used was #n.
On 2010/04/26 18:24:58, gri wrote:
> What does "apply" mean in this context. What is the result?

Done.

http://codereview.appspot.com/1008042/diff/15001/16009#newcode423
src/cmd/godoc/codewalk.go:423: // but it is not implemented.
On 2010/04/26 18:24:58, gri wrote:
> ditto

Done.

http://codereview.appspot.com/1008042/diff/15001/16009#newcode457
src/cmd/godoc/codewalk.go:457: if n--; n == 0 {
On 2010/04/26 18:24:58, gri wrote:
> move the n-- outside the if. as it is, it almost looks like as if it were
> executed conditionally. at the very least, it's confusing

I think this is a nice idiom, like if(n-- == 0) in C.
It shows up elsewhere in the tree too, and not only in code
that I wrote:

./cmd/godoc/codewalk.go:234:			if n--; n == 0 {
./cmd/godoc/codewalk.go:472:			if n--; n == 0 {
./pkg/compress/flate/huffman_code.go:158:		if l.needed--; l.needed == 0 {
./pkg/compress/flate/huffman_code.go:253:		if l.needed--; l.needed == 0 {
./pkg/http/request.go:496:		if nheader++; nheader >= maxHeaderLines {
./pkg/http/response.go:115:		if nheader++; nheader >= maxHeaderLines {
./pkg/json/stream_test.go:91:			if n--; n == 0 {
./pkg/net/dnsmsg.go:349:			if ptr++; ptr > 10 {
./pkg/net/ip.go:402:			if i++; i == len(s) { // can be at end
./pkg/netchan/export.go:152:			if count--; count == 0 {

[rsc, 2010-04-27 05:35:16]

*** Submitted as http://code.google.com/p/go/source/detail?r=46460703635a ***

godoc: add codewalk support

R=adg, gri
CC=golang-dev, r
http://codereview.appspot.com/1008042