code review 4029046: godoc: full text index for whitelisted non-Go files

godoc: full text index for whitelisted non-Go files

[gri, 2011-01-27 03:18:34]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[rsc1, 2011-01-27 03:44:56]

LGTM

http://codereview.appspot.com/4029046/diff/8001/src/cmd/godoc/index.go
File src/cmd/godoc/index.go (right):

http://codereview.appspot.com/4029046/diff/8001/src/cmd/godoc/index.go#newcod...
src/cmd/godoc/index.go:663: file = x.fset.File(token.Pos(base)) //
token.Pos(base) is inside the file
Not sure why this code is needed.  
Won't falling through to the isText(src) body do the same thing?
Spelled slightly differently though, so I'm not sure.

http://codereview.appspot.com/4029046/diff/8001/src/cmd/godoc/index.go#newcod...
src/cmd/godoc/index.go:692: var whitelisted = map[string]int{
if you make this a map to bool with true keys
you can just 

return whitelisted[key]

below

http://codereview.appspot.com/4029046/diff/8001/src/cmd/godoc/index.go#newcod...
src/cmd/godoc/index.go:700: ".text":        0,
can delete.
there are none of these in the tree.

but please add
.goc
.sh
.bash
.out
.py
.js
.xml

all of which exist and could be useful

http://codereview.appspot.com/4029046/diff/8001/src/cmd/godoc/utils.go
File src/cmd/godoc/utils.go (right):

http://codereview.appspot.com/4029046/diff/8001/src/cmd/godoc/utils.go#newcod...
src/cmd/godoc/utils.go:145: // isText returns true if the file has a known
extension indicating a text file
s/isText/isTextFile/

[gri, 2011-01-27 05:49:06]

On Wed, Jan 26, 2011 at 7:44 PM, <rsc@google.com> wrote:

> LGTM
>
>
>
> http://codereview.appspot.com/4029046/diff/8001/src/cmd/godoc/index.go
> File src/cmd/godoc/index.go (right):
>
>
>
http://codereview.appspot.com/4029046/diff/8001/src/cmd/godoc/index.go#newcod...
> src/cmd/godoc/index.go:663: file = x.fset.File(token.Pos(base)) //
> token.Pos(base) is inside the file
> Not sure why this code is needed.
> Won't falling through to the isText(src) body do the same thing?
> Spelled slightly differently though, so I'm not sure.
>

Almost, but the file was already added by ParseFile and must not be added a
2nd time (this should change eventually, when I adjust ParseFile to do the
same as the Scanner which does not add the file to the file set itself
anymore). Added a comment.



>
>
>
http://codereview.appspot.com/4029046/diff/8001/src/cmd/godoc/index.go#newcod...
> src/cmd/godoc/index.go:692: var whitelisted = map[string]int{
> if you make this a map to bool with true keys
> you can just
>
> return whitelisted[key]
>
> below
>
>
>
http://codereview.appspot.com/4029046/diff/8001/src/cmd/godoc/index.go#newcod...
> src/cmd/godoc/index.go:700: ".text":        0,
> can delete.
> there are none of these in the tree.
>
> but please add
> .goc
> .sh
> .bash
> .out
> .py
> .js
> .xml
>
> all of which exist and could be useful
>

done


>
> http://codereview.appspot.com/4029046/diff/8001/src/cmd/godoc/utils.go
> File src/cmd/godoc/utils.go (right):
>
>
>
http://codereview.appspot.com/4029046/diff/8001/src/cmd/godoc/utils.go#newcod...
> src/cmd/godoc/utils.go:145: // isText returns true if the file has a
> known extension indicating a text file
> s/isText/isTextFile/
>
> done

>
> http://codereview.appspot.com/4029046/
>

[gri, 2011-01-27 05:49:34]

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

godoc: full text index for whitelisted non-Go files

R=rsc
CC=golang-dev
http://codereview.appspot.com/4029046

Committer: Robert Griesemer <gri@golang.org>