code review 6453094: godoc: adjust import path for directories with multiple...

godoc: report error for directories with multiple packages

Fixes issue 3922.

[gri, 2012-08-07 21:28:30]

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

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

[adg, 2012-08-08 00:24:32]

I don't think this is right. I updated the issue http://golang.org/issue/3922

[rsc, 2012-08-08 19:41:33]

I suggest we assume that we can let go/build tell us about the one package in
the directory. Then all the tools are consistent.

Environments with non-standard usages can always implement a vfs in godoc to
provide a simulation of the standard tree.

[gri, 2012-08-08 20:36:00]

The question remains: what should godoc do in case of multiple
packages in the same directory?

It sounds like the answer is: show an error. This would then (down the
road) fit with a changed parser.ParseDir signature which would not
return a map of packages found but a single package if there is only
one, and an error if there is more than one.

Comments?
- gri

On Wed, Aug 8, 2012 at 12:41 PM,  <rsc@golang.org> wrote:
> I suggest we assume that we can let go/build tell us about the one
> package in the directory. Then all the tools are consistent.
>
> Environments with non-standard usages can always implement a vfs in
> godoc to provide a simulation of the standard tree.
>
>
> http://codereview.appspot.com/6453094/

[gri, 2012-08-08 22:01:13]

PTAL.

http://codereview.appspot.com/6453094

- godoc reports error for directories with multiple packages
(excluding test and example packages)
- remove functionality to select a specific package in a directory

On Wed, Aug 8, 2012 at 1:35 PM, Robert Griesemer <gri@golang.org> wrote:
> The question remains: what should godoc do in case of multiple
> packages in the same directory?
>
> It sounds like the answer is: show an error. This would then (down the
> road) fit with a changed parser.ParseDir signature which would not
> return a map of packages found but a single package if there is only
> one, and an error if there is more than one.
>
> Comments?
> - gri
>
> On Wed, Aug 8, 2012 at 12:41 PM,  <rsc@golang.org> wrote:
>> I suggest we assume that we can let go/build tell us about the one
>> package in the directory. Then all the tools are consistent.
>>
>> Environments with non-standard usages can always implement a vfs in
>> godoc to provide a simulation of the standard tree.
>>
>>
>> http://codereview.appspot.com/6453094/

[rsc, 2012-08-09 16:39:16]

http://codereview.appspot.com/6453094/diff/7005/src/cmd/godoc/godoc.go
File src/cmd/godoc/godoc.go (right):

http://codereview.appspot.com/6453094/diff/7005/src/cmd/godoc/godoc.go#newcod...
src/cmd/godoc/godoc.go:915: pkgs, err := parseDir(fset, abspath, filter)
I think we can go even further here. Above, the code tries to continue on if
ctxt.ImportDir fails. I suggest that if ctxt.ImportDir fails, then stop and show
an error then.

When we get here, the set of files to read is exactly dir.GoFiles +
dir.CgoFiles.

I don't believe ast/parser need ever change, by the way. The single directory
single package convention is enforced by the go tools - cmd/go, go/build,
cmd/godoc - but I don't think it should be enforced by the parser itself.

[gri, 2012-08-09 17:05:40]

http://codereview.appspot.com/6453094/diff/7005/src/cmd/godoc/godoc.go
File src/cmd/godoc/godoc.go (right):

http://codereview.appspot.com/6453094/diff/7005/src/cmd/godoc/godoc.go#newcod...
src/cmd/godoc/godoc.go:915: pkgs, err := parseDir(fset, abspath, filter)
On 2012/08/09 16:39:16, rsc wrote:
> I think we can go even further here. Above, the code tries to continue on if
> ctxt.ImportDir fails. I suggest that if ctxt.ImportDir fails, then stop and
show
> an error then.
> 
> When we get here, the set of files to read is exactly dir.GoFiles +
> dir.CgoFiles.
> 
> I don't believe ast/parser need ever change, by the way. The single directory
> single package convention is enforced by the go tools - cmd/go, go/build,
> cmd/godoc - but I don't think it should be enforced by the parser itself.
> 

That won't work w/o quite a bit more change of the logic. getPageInfo is also
called for directories which don't contain any .go files (e.g. src/pkg) and in
that case it produces a list of directories. build.ImportDir returns an error in
that case and godoc won't show anything. I prefer to leave it as is for now.

At some point, with all the conventions in place now, it may make sense to do a
larger round of cleanups in godoc. Separate CL.

[rsc, 2012-08-09 18:49:42]

LGTM

[gri, 2012-08-09 23:10:49]

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

godoc: report error for directories with multiple packages

Fixes issue 3922.

R=rsc, adg
CC=golang-dev
http://codereview.appspot.com/6453094