FAQ
Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go


Description:
cmd/godoc: handle file name arguments better

This change allows you to refer to the directory "foo" relative
to current directory as "foo" or "./foo".

Furthermore, it discovers if "foo" resides under an GOPATH or GOROOT
and displays the correct import path. For example,
cd $GOROOT/src/pkg/archive
godoc zip
will display the docs for "archive/zip" with the correct import path.

Fixes issue 4330.

Please review this at https://codereview.appspot.com/7132044/

Affected files:
M src/cmd/godoc/doc.go
M src/cmd/godoc/main.go


Index: src/cmd/godoc/doc.go
===================================================================
--- a/src/cmd/godoc/doc.go
+++ b/src/cmd/godoc/doc.go
@@ -9,15 +9,20 @@
It has two modes.

Without the -http flag, it runs in command-line mode and prints plain text
-documentation to standard output and exits. If both a library package and
-a command with the same name exists, using the prefix cmd/ will force
-documentation on the command rather than the library package. If the -src
-flag is specified, godoc prints the exported interface of a package in Go
-source form, or the implementation of a specific exported language entity:
+documentation to standard output and exits.
+
+Its first non-flag argument must be an import path or an absolute or
relative
+file system path to a Go package or command. If both a standard library
package
+and a Go tool command with the same name exists, using the prefix cmd/ will
+force documentation on the command rather than the library package.
+
+If the -src flag is specified, godoc prints the exported interface of a
package
+in Go source form, or the implementation of a specific exported language
entity:

godoc fmt # documentation for package fmt
godoc fmt Printf # documentation for fmt.Printf
godoc cmd/go # force documentation for the go command
+ godoc . # docs for the package in the current directory
godoc -src fmt # fmt package interface in Go source form
godoc -src fmt Printf # implementation of fmt.Printf

Index: src/cmd/godoc/main.go
===================================================================
--- a/src/cmd/godoc/main.go
+++ b/src/cmd/godoc/main.go
@@ -339,22 +339,33 @@
var forceCmd bool
var abspath, relpath string
if filepath.IsAbs(path) {
+ // Absolute path takes precedence.
+ // (the go command invokes godoc w/ absolute paths; don't override)
fs.Bind(target, OS(path), "/", bindReplace)
abspath = target
- } else if build.IsLocalImport(path) {
+ } else if strings.HasPrefix(path, cmdPrefix) {
+ // Path "cmd/foo" refers to go command "foo".
+ path = path[len(cmdPrefix):]
+ forceCmd = true
+ } else if bp, _ := build.Import(path, "",
build.FindOnly); !build.IsLocalImport(path) && bp.Dir != "" &&
bp.ImportPath != "" {
+ // Packages in $GOROOT/src/pkg or GOPATH directories.
+ fs.Bind(target, OS(bp.Dir), "/", bindReplace)
+ abspath = target
+ relpath = bp.ImportPath
+ } else if fi, err := os.Stat(path); err == nil && fi.IsDir() {
+ // Path is a directory relative to the CWD.
cwd, _ := os.Getwd() // ignore errors
path = filepath.Join(cwd, path)
fs.Bind(target, OS(path), "/", bindReplace)
abspath = target
- } else if strings.HasPrefix(path, cmdPrefix) {
- path = path[len(cmdPrefix):]
- forceCmd = true
- } else if bp, _ := build.Import(path, "", build.FindOnly); bp.Dir != ""
&& bp.ImportPath != "" {
- fs.Bind(target, OS(bp.Dir), "/", bindReplace)
- abspath = target
- relpath = bp.ImportPath
- } else {
- abspath = pathpkg.Join(pkgHandler.fsRoot, path)
+ relpath = path
+ // If the directory is inside GOPATH,
+ // use its actual import path as relpath.
+ for _, root := range build.Default.SrcDirs() {
+ if p, ok := hasSubdir(root, path); ok {
+ relpath = p
+ }
+ }
}
if relpath == "" {
relpath = abspath
@@ -373,37 +384,41 @@
mode |= showSource
}

- // first, try as package unless forced as command
+ // First, try as package unless forced as command.
var info PageInfo
if !forceCmd {
info = pkgHandler.getPageInfo(abspath, relpath, mode)
}

- // second, try as command unless the path is absolute
- // (the go command invokes godoc w/ absolute paths; don't override)
+ // Also try as command if path is not absolute.
var cinfo PageInfo
if !filepath.IsAbs(path) {
abspath = pathpkg.Join(cmdHandler.fsRoot, path)
cinfo = cmdHandler.getPageInfo(abspath, relpath, mode)
}

- // determine what to use
+ // Choose whether to use info or cinfo.
if info.IsEmpty() {
- if !cinfo.IsEmpty() {
- // only cinfo exists - switch to cinfo
- info = cinfo
- }
+ // info doesn't exist - use cinfo.
+ info = cinfo
} else if !cinfo.IsEmpty() {
- // both info and cinfo exist - use cinfo if info
- // contains only subdirectory information
+ // Both info and cinfo exist.
if info.PAst == nil && info.PDoc == nil {
+ // info contains only subdirectory information - use cinfo.
+ // (Choose cmd/go over pkg/go for path "go".)
info = cinfo
} else {
+ // Both info and cinfo contain docs - use info.
+ // But tell user how to get the command docs.
fmt.Printf("use 'godoc %s%s' for documentation on the %s command \n\n",
cmdPrefix, relpath, relpath)
}
}

if info.Err != nil {
+ if os.IsNotExist(info.Err) {
+ // Show a friendlier error than the one returned by os.Open.
+ log.Fatalf("package not found: %v", path)
+ }
log.Fatalf("%v", info.Err)
}
if info.PDoc != nil && info.PDoc.ImportPath == target {
@@ -455,6 +470,29 @@
}
}

+// hasSubdir reports whether dir is a subdirectory of
+// (perhaps multiple levels below) root.
+// If so, hasSubdir sets rel to a slash-separated path that
+// can be joined to root to produce a path equivalent to dir.
+func hasSubdir(root, dir string) (rel string, ok bool) {
+ if p, err := filepath.EvalSymlinks(root); err == nil {
+ root = p
+ }
+ if p, err := filepath.EvalSymlinks(dir); err == nil {
+ dir = p
+ }
+ const sep = string(filepath.Separator)
+ root = filepath.Clean(root)
+ if !strings.HasSuffix(root, sep) {
+ root += sep
+ }
+ dir = filepath.Clean(dir)
+ if !strings.HasPrefix(dir, root) {
+ return "", false
+ }
+ return filepath.ToSlash(dir[len(root):]), true
+}
+
// An httpWriter is an http.ResponseWriter writing to a bytes.Buffer.
type httpWriter struct {
bytes.Buffer

Search Discussions

  • Minux at Jan 16, 2013 at 12:02 pm
    the behavior for commands seems to be inconsistent with that of normal
    packages.

    try this:
    godoc cmd/godoc # -> command document
    cd $GOROOT/src/cmd
    godoc godoc # -> package document

    godoc archive/zip # -> package document
    cd $GOROOT/src/pkg/archive
    godoc zip # -> also package document

    btw, what do you think it's the best way to say package document for a
    command?
    i used to do something like this: godoc ./cmd/godoc to view the package
    document
    for cmd/godoc.
  • Andrew Gerrand at Jan 17, 2013 at 3:22 am

    On 16 January 2013 21:58, minux wrote:

    the behavior for commands seems to be inconsistent with that of normal
    packages.

    try this:
    godoc cmd/godoc # -> command document
    cd $GOROOT/src/cmd
    godoc godoc # -> package document

    godoc archive/zip # -> package document
    cd $GOROOT/src/pkg/archive
    godoc zip # -> also package document
    Ugh! It is so annoying that commands and packages are documented
    differently.

    I've made some more changes. My goal with this CL was to clean things up a
    bit, but I think I've merely maintained the status quo. :/

    btw, what do you think it's the best way to say package document for a
    command?
    i used to do something like this: godoc ./cmd/godoc to view the package
    document
    for cmd/godoc.
    godoc cmd/godoc
    ?
  • Andrew Gerrand at Jan 17, 2013 at 5:28 am
  • Russ Cox at Jan 17, 2013 at 2:12 pm
    I am not sure this is a good idea. I am uncomfortable with the
    ambiguity this introduces. What if there is a subdirectory called math
    and I godoc math? Which one do I get? It has to be the standard math
    since otherwise there is no way to name the standard math. But then
    people will complain that the subdirectory doesn't work and still have
    to learn about ./math. Perhaps we can resolve issue 4330 by making
    Rob's example print:

    % godoc now # WHY DOESN'T THIS WORK?
    godoc: cannot find import path "now"
    For the local directory, use godoc ./now.
    %

    (The second line would print only if the directory existed.)

    Russ
  • Andrew Gerrand at Jan 17, 2013 at 11:25 pm

    On 18 January 2013 01:12, Russ Cox wrote:

    What if there is a subdirectory called math
    and I godoc math? Which one do I get?
    You get the standard library's math. The order of precedence for
    non-absolute paths is: GOROOT, GOPATH, path relative to CWD. This seems the
    least surprising order to me. I'm not sure people will complain, as this
    change merely gives them a convenience (naming relative paths without
    starting them with dot) that they didn't have before.

    The other important thing this CL does is make sure the package's import
    path is displayed correctly if that package is inside GOROOT or GOPATH.
    That's an important improvement.

    It's easy enough to alter this change to remove the former functionality
    change and add the message you suggest, and keep the latter. What do you
    think?
  • Russ Cox at Jan 18, 2013 at 4:28 pm

    You get the standard library's math. The order of precedence for
    non-absolute paths is: GOROOT, GOPATH, path relative to CWD. This seems the
    least surprising order to me. I'm not sure people will complain, as this
    change merely gives them a convenience (naming relative paths without
    starting them with dot) that they didn't have before.
    I am complaining already. :-)

    People complaining is probably not a good metric for whether it's a
    good design. Convenience today is confusion tomorrow.
    The other important thing this CL does is make sure the package's import
    path is displayed correctly if that package is inside GOROOT or GOPATH.
    That's an important improvement.

    It's easy enough to alter this change to remove the former functionality
    change and add the message you suggest, and keep the latter. What do you
    think?
    That sounds fine: remove the ambiguity, add a warning if nothing is
    found but ./ would fix that, and fix the import path.

    Russ
  • Adg at Jan 18, 2013 at 11:05 pm
    Hello golang-dev@googlegroups.com, minux.ma@gmail.com, rsc@golang.org
    (cc: golang-dev@googlegroups.com),

    Please take another look.


    https://codereview.appspot.com/7132044/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedJan 16, '13 at 9:51a
activeJan 18, '13 at 11:05p
posts8
users3
websitegolang.org

3 users in discussion

Adg: 5 posts Russ Cox: 2 posts Minux: 1 post

People

Translate

site design / logo © 2022 Grokbase