Code review - Issue 4572065: code review 4572065: godoc: replace direct OS file system accesses in favorhttps://codereview.appspot.com/2011-06-15T21:06:44+00:00rietveld
Message from unknown
2011-06-15T00:03:58+00:00griurn:md5:e2e580f875afe4ad668a1e0594bcc40e
Message from unknown
2011-06-15T00:04:11+00:00griurn:md5:6da2e007f5a14854ddf94f33c5b2d77c
Message from unknown
2011-06-15T00:11:38+00:00griurn:md5:959c50d25a97b1269c06004164664151
Message from unknown
2011-06-15T00:46:08+00:00griurn:md5:309758c3aa233f42158f4487bc0d7dee
Message from unknown
2011-06-15T00:57:10+00:00griurn:md5:5498484c8b108094e93834f9346188b4
Message from gri@golang.org
2011-06-15T00:57:14+00:00griurn:md5:bb5dd3459970ddb34511fc3e7b87cbfe
Hello rsc@golang.org, r@golang.org (cc: golang-dev@googlegroups.com),
I'd like you to review this change to
https://go.googlecode.com/hg/
Message from rsc@golang.org
2011-06-15T02:46:47+00:00rscurn:md5:f603a4c40c7d4c4c34c59988ef47f647
http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/filesystem.go
File src/cmd/godoc/filesystem.go (right):
http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/filesystem.go#newcode17
src/cmd/godoc/filesystem.go:17: type FileInfo interface {
I don't believe this makes sense. os.FileInfo is already written to
be portable, meaning operating system-independent, and it is
pure exported data. I suggest using *os.FileInfo below, and
then a level of wrapping and a bunch of changes all disappear.
http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/filesystem.go#newcode64
src/cmd/godoc/filesystem.go:64: func (fs osFS) Open(path string) (io.ReadCloser, os.Error) {
func (osFS)
http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/godoc.go
File src/cmd/godoc/godoc.go (left):
http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/godoc.go#oldcode824
src/cmd/godoc/godoc.go:824: for _, d := range list {
It seems odd to redefine the interface to be different from os.
I would keep this code.
http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/index.go
File src/cmd/godoc/index.go (right):
http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/index.go#newcode909
src/cmd/godoc/index.go:909: err = os.ErrorString("all query parts must be identifiers")
There are a bunch of these, and they are unrelated to this change.
Please revert them so that the diffs are clearer.
I pre-LGTM any CL containing just NewError -> ErrorString
in godoc if you want to convert them all.
http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/utils.go
File src/cmd/godoc/utils.go (right):
http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/utils.go#newcode47
src/cmd/godoc/utils.go:47: // TODO(gri) must use the underlying file system here, not os
This could be clearer. I think of 'os' as the underlying file system.
Message from unknown
2011-06-15T20:07:36+00:00griurn:md5:049400a5d32a8eadc46348513d4cd346
Message from gri@golang.org
2011-06-15T20:12:03+00:00griurn:md5:579bfd460ba99a17668b2cc53f7e0093
PTAL
http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/filesystem.go
File src/cmd/godoc/filesystem.go (right):
http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/filesystem.go#newcode17
src/cmd/godoc/filesystem.go:17: type FileInfo interface {
On 2011/06/15 02:46:47, rsc wrote:
> I don't believe this makes sense. os.FileInfo is already written to
> be portable, meaning operating system-independent, and it is
> pure exported data. I suggest using *os.FileInfo below, and
> then a level of wrapping and a bunch of changes all disappear.
>
I was thinking about doing that, but I believe that makes things more complicated. os.FileInfo may be portable but there's a lot of information in there that I don't care about (and may not even exist), and I don't want to setup (or even just allocate the space for) an os.FileInfo when the "files" are coming from elsewhere (say via a zip file). This is very light-weight, cheap, and abstract. For osFS, there's very little code needed, and for another implementation I am not bound to set up a (partial) os.FileInfo.
http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/filesystem.go#newcode64
src/cmd/godoc/filesystem.go:64: func (fs osFS) Open(path string) (io.ReadCloser, os.Error) {
On 2011/06/15 02:46:47, rsc wrote:
> func (osFS)
Done.
http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/godoc.go
File src/cmd/godoc/godoc.go (left):
http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/godoc.go#oldcode824
src/cmd/godoc/godoc.go:824: for _, d := range list {
On 2011/06/15 02:46:47, rsc wrote:
> It seems odd to redefine the interface to be different from os.
> I would keep this code.
Again, this requires os.FileInfo, or some settable size. The size returned from os.Stat is really platform specific. With the new FileInfo, this size is already 0 and so this is not needed. Seems cleaner to me.
http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/index.go
File src/cmd/godoc/index.go (right):
http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/index.go#newcode909
src/cmd/godoc/index.go:909: err = os.ErrorString("all query parts must be identifiers")
On 2011/06/15 02:46:47, rsc wrote:
> There are a bunch of these, and they are unrelated to this change.
> Please revert them so that the diffs are clearer.
>
> I pre-LGTM any CL containing just NewError -> ErrorString
> in godoc if you want to convert them all.
>
Done.
http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/utils.go
File src/cmd/godoc/utils.go (right):
http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/utils.go#newcode47
src/cmd/godoc/utils.go:47: // TODO(gri) must use the underlying file system here, not os
On 2011/06/15 02:46:47, rsc wrote:
> This could be clearer. I think of 'os' as the underlying file system.
Done.
Message from rsc@golang.org
2011-06-15T20:31:26+00:00rscurn:md5:53689e3b1cb43048082671c47647341c
LGTM
Message from unknown
2011-06-15T21:06:29+00:00griurn:md5:7c094fe3a9e60421a15109924d98df62
Message from gri@golang.org
2011-06-15T21:06:44+00:00griurn:md5:cc3ed1db67d739536ec82c0990054301
*** Submitted as http://code.google.com/p/go/source/detail?r=3a5f42c956e0 ***
godoc: replace direct OS file system accesses in favor
of accesses via a FileSystem interface.
Preparation for appengine version which gets its files
via a snapshot or zip file and uses a corresponding
FileSystem implementation.
R=rsc, r
CC=golang-dev
http://codereview.appspot.com/4572065