code review 5699072: godoc: don't show directories w/o packages in flat dir mode

godoc: don't show directories w/o packages in flat dir mode

The main change is simple: Both the Directory and DirEntry
struct have an extra field 'HasPkg' indicating whether the
directory contains any package files. The remaining changes
are more comments and adjustments to the template files.

Fixes issue 3121.

[gri, 2012-02-24 22:27:17]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2012-02-26 08:22:53]

http://codereview.appspot.com/5699072/diff/3001/src/cmd/godoc/dirtrees.go
File src/cmd/godoc/dirtrees.go (right):

http://codereview.appspot.com/5699072/diff/3001/src/cmd/godoc/dirtrees.go#new...
src/cmd/godoc/dirtrees.go:29: HasPkg   bool         // true if the directory
contains at least one package
package or command? or just package?

if the former (which I think it is?), I'd name this HasGoCode or HasCode maybe.

http://codereview.appspot.com/5699072/diff/3001/src/cmd/godoc/dirtrees.go#new...
src/cmd/godoc/dirtrees.go:66: return &Directory{depth, path, name, false, "",
nil}
I think this passed the reasonable limit for tagless composite literals a few
zero values ago.

http://codereview.appspot.com/5699072/diff/3001/src/cmd/godoc/dirtrees.go#new...
src/cmd/godoc/dirtrees.go:149: return &Directory{depth, path, name, hasPkgFiles,
synopsis, dirs}
I'd spell this out too.  Too many of these share the same type (string), making
me nervous that the compiler won't catch things if you decide to reorder the
struct declaration for clarity.

[Sameer Ajmani, 2012-02-27 01:44:15]

http://codereview.appspot.com/5699072/diff/3001/lib/godoc/package.txt
File lib/godoc/package.txt (right):

http://codereview.appspot.com/5699072/diff/3001/lib/godoc/package.txt#newcode80
lib/godoc/package.txt:80: {{.Path}}{{end}}{{end}}
It's hard to see what each {{end}} matches here.  Can you break this across a
few lines and indent to make this clearer?

http://codereview.appspot.com/5699072/diff/3001/src/cmd/godoc/dirtrees.go
File src/cmd/godoc/dirtrees.go (right):

http://codereview.appspot.com/5699072/diff/3001/src/cmd/godoc/dirtrees.go#new...
src/cmd/godoc/dirtrees.go:29: HasPkg   bool         // true if the directory
contains at least one package
On 2012/02/26 08:22:53, bradfitz wrote:
> package or command? or just package?
> 
> if the former (which I think it is?), I'd name this HasGoCode or HasCode
maybe.

I'd color it HasFiles.

http://codereview.appspot.com/5699072/diff/3001/src/cmd/godoc/dirtrees.go#new...
src/cmd/godoc/dirtrees.go:66: return &Directory{depth, path, name, false, "",
nil}
On 2012/02/26 08:22:53, bradfitz wrote:
> I think this passed the reasonable limit for tagless composite literals a few
> zero values ago.

+1

[gri, 2012-02-27 18:49:40]

http://codereview.appspot.com/5699072/diff/3001/lib/godoc/package.txt
File lib/godoc/package.txt (right):

http://codereview.appspot.com/5699072/diff/3001/lib/godoc/package.txt#newcode80
lib/godoc/package.txt:80: {{.Path}}{{end}}{{end}}
On 2012/02/27 01:44:15, Sameer Ajmani wrote:
> It's hard to see what each {{end}} matches here.  Can you break this across a
> few lines and indent to make this clearer?

No.

This is a problem with templates for textual output, every newline becomes part
of the template. The generated output won;t look right.

http://codereview.appspot.com/5699072/diff/3001/src/cmd/godoc/dirtrees.go
File src/cmd/godoc/dirtrees.go (right):

http://codereview.appspot.com/5699072/diff/3001/src/cmd/godoc/dirtrees.go#new...
src/cmd/godoc/dirtrees.go:29: HasPkg   bool         // true if the directory
contains at least one package
On 2012/02/26 08:22:53, bradfitz wrote:
> package or command? or just package?
> 
> if the former (which I think it is?), I'd name this HasGoCode or HasCode
maybe.

As the comments says: at least one package. And a command is a package.

http://codereview.appspot.com/5699072/diff/3001/src/cmd/godoc/dirtrees.go#new...
src/cmd/godoc/dirtrees.go:29: HasPkg   bool         // true if the directory
contains at least one package
On 2012/02/27 01:44:15, Sameer Ajmani wrote:
> On 2012/02/26 08:22:53, bradfitz wrote:
> > package or command? or just package?
> > 
> > if the former (which I think it is?), I'd name this HasGoCode or HasCode
> maybe.
> 
> I'd color it HasFiles.

HasFiles is misleading. This is about .go files who build a package, only.

http://codereview.appspot.com/5699072/diff/3001/src/cmd/godoc/dirtrees.go#new...
src/cmd/godoc/dirtrees.go:66: return &Directory{depth, path, name, false, "",
nil}
On 2012/02/27 01:44:15, Sameer Ajmani wrote:
> On 2012/02/26 08:22:53, bradfitz wrote:
> > I think this passed the reasonable limit for tagless composite literals a
few
> > zero values ago.
> 
> +1

Done.

http://codereview.appspot.com/5699072/diff/3001/src/cmd/godoc/dirtrees.go#new...
src/cmd/godoc/dirtrees.go:66: return &Directory{depth, path, name, false, "",
nil}
On 2012/02/26 08:22:53, bradfitz wrote:
> I think this passed the reasonable limit for tagless composite literals a few
> zero values ago.

Done.

http://codereview.appspot.com/5699072/diff/3001/src/cmd/godoc/dirtrees.go#new...
src/cmd/godoc/dirtrees.go:149: return &Directory{depth, path, name, hasPkgFiles,
synopsis, dirs}
On 2012/02/26 08:22:53, bradfitz wrote:
> I'd spell this out too.  Too many of these share the same type (string),
making
> me nervous that the compiler won't catch things if you decide to reorder the
> struct declaration for clarity.

Done.

[gri, 2012-02-27 18:50:16]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, sameer@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-02-27 19:11:18]

LGTM

On Mon, Feb 27, 2012 at 10:50 AM, <gri@golang.org> wrote:

> Hello golang-dev@googlegroups.com, bradfitz@golang.org,
> sameer@golang.org (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
http://codereview.appspot.com/**5699072/<http://codereview.appspot.com/5699072/>
>

[gri, 2012-02-27 19:18:03]

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

godoc: don't show directories w/o packages in flat dir mode

The main change is simple: Both the Directory and DirEntry
struct have an extra field 'HasPkg' indicating whether the
directory contains any package files. The remaining changes
are more comments and adjustments to the template files.

Fixes issue 3121.

R=golang-dev, bradfitz, sameer
CC=golang-dev
http://codereview.appspot.com/5699072