Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(325)

Issue 5711058: code review 5711058: godoc: support $GOPATH, simplify file system code (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 2 months ago by rsc
Modified:
7 years, 2 months ago
Reviewers:
CC:
gri, r, r2, rsc1, golang-dev
Visibility:
Public.

Description

godoc: support $GOPATH, simplify file system code The motivation for this CL is to support $GOPATH well. Since we already have a FileSystem interface, implement a Plan 9-style name space. Bind each of the $GOPATH src directories onto the $GOROOT src/pkg directory: now everything is laid out exactly like a normal $GOROOT and needs very little special case code. The filter files are no longer used (by us), so I think they can just be deleted. Similarly, the Mapping code and the FileSystem interface were two different ways to accomplish the same end, so delete the Mapping code. Within the implementation, since FileSystem is defined to be slash-separated, use package path consistently, leaving path/filepath only for manipulating operating system paths. I kept the -path flag, but I think it can be deleted too. Fixes issue 2234. Fixes issue 3046.

Patch Set 1 #

Patch Set 2 : diff -r 60df364bae52 https://go.googlecode.com/hg/ #

Total comments: 7

Patch Set 3 : diff -r ab23f67d6786 https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r ab23f67d6786 https://go.googlecode.com/hg/ #

Patch Set 5 : diff -r 54f0be418491 https://code.google.com/p/go/ #

Patch Set 6 : diff -r 54f0be418491 https://code.google.com/p/go/ #

Total comments: 6

Patch Set 7 : diff -r 54f0be418491 https://code.google.com/p/go/ #

Patch Set 8 : diff -r 54f0be418491 https://code.google.com/p/go/ #

Patch Set 9 : diff -r 54f0be418491 https://code.google.com/p/go/ #

Total comments: 15

Patch Set 10 : diff -r 85bd2ea73846 https://code.google.com/p/go/ #

Patch Set 11 : diff -r 85bd2ea73846 https://code.google.com/p/go/ #

Patch Set 12 : diff -r 8d4fd582202b https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+637 lines, -799 lines) Patch
M lib/godoc/package.html View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M src/cmd/godoc/appinit.go View 1 2 3 4 5 2 chunks +1 line, -5 lines 0 comments Download
M src/cmd/godoc/codewalk.go View 1 3 chunks +3 lines, -3 lines 0 comments Download
M src/cmd/godoc/dirtrees.go View 1 9 chunks +22 lines, -13 lines 0 comments Download
M src/cmd/godoc/filesystem.go View 1 2 3 4 5 6 7 8 10 11 3 chunks +488 lines, -14 lines 0 comments Download
M src/cmd/godoc/godoc.go View 1 2 3 4 5 11 29 chunks +53 lines, -255 lines 0 comments Download
R src/cmd/godoc/httpzip.go View 1 1 chunk +0 lines, -190 lines 0 comments Download
M src/cmd/godoc/index.go View 1 4 chunks +4 lines, -4 lines 0 comments Download
M src/cmd/godoc/main.go View 1 2 3 4 5 10 11 8 chunks +30 lines, -29 lines 0 comments Download
R src/cmd/godoc/mapping.go View 1 1 chunk +0 lines, -202 lines 0 comments Download
M src/cmd/godoc/parser.go View 1 2 chunks +2 lines, -2 lines 0 comments Download
M src/cmd/godoc/utils.go View 1 3 chunks +2 lines, -77 lines 0 comments Download
M src/cmd/godoc/zip.go View 1 4 chunks +31 lines, -4 lines 0 comments Download

Messages

Total messages: 15
rsc
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
7 years, 2 months ago (2012-03-01 22:08:36 UTC) #1
gri
Some preliminary comments. Still digesting this. Please add better documentation to the new filesystem ops ...
7 years, 2 months ago (2012-03-01 23:28:07 UTC) #2
rsc
PTAL I made the suggested changes and added a few large comments and worked examples ...
7 years, 2 months ago (2012-03-02 03:29:51 UTC) #3
rsc1
http://codereview.appspot.com/5711058/diff/11001/src/cmd/godoc/filesystem.go File src/cmd/godoc/filesystem.go (right): http://codereview.appspot.com/5711058/diff/11001/src/cmd/godoc/filesystem.go#newcode349 src/cmd/godoc/filesystem.go:349: return ns.stat(path, FileSystem.Stat) By the way, first time I've ...
7 years, 2 months ago (2012-03-02 03:38:09 UTC) #4
r
LVGTM http://codereview.appspot.com/5711058/diff/11001/src/cmd/godoc/filesystem.go File src/cmd/godoc/filesystem.go (right): http://codereview.appspot.com/5711058/diff/11001/src/cmd/godoc/filesystem.go#newcode148 src/cmd/godoc/filesystem.go:148: // {old: "/src/pkg", fs: OS(`d:\Work2`), new: "/src"}, all ...
7 years, 2 months ago (2012-03-02 04:16:09 UTC) #5
rsc
On Thu, Mar 1, 2012 at 23:16, <r@golang.org> wrote: > all these examples would be ...
7 years, 2 months ago (2012-03-02 04:20:15 UTC) #6
r2
On Mar 2, 2012, at 3:20 PM, Russ Cox wrote: > On Thu, Mar 1, ...
7 years, 2 months ago (2012-03-02 04:21:16 UTC) #7
rsc
I expanded the comment to show the fs+new reading: // Given this name space, a ...
7 years, 2 months ago (2012-03-02 04:25:22 UTC) #8
r2
On Mar 2, 2012, at 3:25 PM, Russ Cox wrote: > I expanded the comment ...
7 years, 2 months ago (2012-03-02 04:28:53 UTC) #9
gri
New comments are great, but I still have some questions. http://codereview.appspot.com/5711058/diff/12018/src/cmd/godoc/filesystem.go File src/cmd/godoc/filesystem.go (right): http://codereview.appspot.com/5711058/diff/12018/src/cmd/godoc/filesystem.go#newcode29 ...
7 years, 2 months ago (2012-03-02 18:59:33 UTC) #10
rsc1
PTAL http://codereview.appspot.com/5711058/diff/12018/src/cmd/godoc/filesystem.go File src/cmd/godoc/filesystem.go (right): http://codereview.appspot.com/5711058/diff/12018/src/cmd/godoc/filesystem.go#newcode29 src/cmd/godoc/filesystem.go:29: // system, which helps keep things simple. On ...
7 years, 2 months ago (2012-03-02 19:24:56 UTC) #11
gri
LGTM http://codereview.appspot.com/5711058/diff/12018/src/cmd/godoc/godoc.go File src/cmd/godoc/godoc.go (left): http://codereview.appspot.com/5711058/diff/12018/src/cmd/godoc/godoc.go#oldcode725 src/cmd/godoc/godoc.go:725: PkgRoots []string On 2012/03/02 19:24:56, rsc1 wrote: > ...
7 years, 2 months ago (2012-03-02 19:56:03 UTC) #12
r
LGTM just in case
7 years, 2 months ago (2012-03-02 21:10:52 UTC) #13
rsc1
Thanks. My plan is to wait to submit this until after Andrew cuts the weekly.
7 years, 2 months ago (2012-03-02 22:28:25 UTC) #14
rsc
7 years, 2 months ago (2012-03-05 15:02:51 UTC) #15
*** Submitted as http://code.google.com/p/go/source/detail?r=ea5cd2b9ca6c ***

godoc: support $GOPATH, simplify file system code

The motivation for this CL is to support $GOPATH well.
Since we already have a FileSystem interface, implement a
Plan 9-style name space.  Bind each of the $GOPATH src
directories onto the $GOROOT src/pkg directory: now
everything is laid out exactly like a normal $GOROOT and
needs very little special case code.

The filter files are no longer used (by us), so I think they
can just be deleted.  Similarly, the Mapping code and the
FileSystem interface were two different ways to accomplish
the same end, so delete the Mapping code.

Within the implementation, since FileSystem is defined to be
slash-separated, use package path consistently, leaving
path/filepath only for manipulating operating system paths.

I kept the -path flag, but I think it can be deleted too.

Fixes issue 2234.
Fixes issue 3046.

R=gri, r, r, rsc
CC=golang-dev
http://codereview.appspot.com/5711058
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b