code review 7341053: cmd/godoc: add support for display Notes parsed by pkg/...

cmd/godoc: add support for display Notes parsed by pkg/go/doc
pkg/go/doc: move BUG notes from Package.Bugs to the general Package.Notes field.
Removing .Bugs would break existing code so it's left in for now.

[cnicolaou, 2013-02-20 00:06:56]

Hello golang-dev@googlegroups.com,

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

[gburd, 2013-02-20 03:41:59]

This change breaks code that uses the Package Bugs field.

[cnicolaou, 2013-02-20 05:19:01]

On 2013/02/20 03:41:59, gburd wrote:
> This change breaks code that uses the Package Bugs field.

The .Bugs field is still there specifically so that no code will fail to
compile. The .Bugs field is not populated which seems like a reasonable step
towards using the new functionality and retiring the old.

[gburd, 2013-02-20 06:05:41]

The Bugs field must be populated to honor the compatibility guarantee that
programs will continue to run correctly.

https://codereview.appspot.com/7341053/diff/1001/lib/godoc/package.html
File lib/godoc/package.html (right):

https://codereview.appspot.com/7341053/diff/1001/lib/godoc/package.html#newco...
lib/godoc/package.html:168: {{range $marker, $content := .}}
Sort the keys so that notes are displayed in a consistent order.

https://codereview.appspot.com/7341053/diff/1001/lib/godoc/package.html#newco...
lib/godoc/package.html:169: <h2 id="pkg-{{$marker}}">{{noteTitle $marker |
html}}s</h2>
In browsers with case-insensitive element ids, this id can clash with other ids
in the document. Consider using <h2 id="pkg-note-{{$marker}}">.

[cnicolaou, 2013-02-20 19:50:10]

ok, done. I've populated the .Bugs field and made sure the unittest covers this
case.

https://codereview.appspot.com/7341053/diff/1001/lib/godoc/package.html
File lib/godoc/package.html (right):

https://codereview.appspot.com/7341053/diff/1001/lib/godoc/package.html#newco...
lib/godoc/package.html:168: {{range $marker, $content := .}}
On 2013/02/20 06:05:41, gburd wrote:
> Sort the keys so that notes are displayed in a consistent order.

if I'm reading the template documentation correctly, then the template package
already does so. Here's the wording:

{range pipeline}} T1 {{end}}
	The value of the pipeline must be an array, slice, or map. If
	the value of the pipeline has length zero, nothing is output;
	otherwise, dot is set to the successive elements of the array,
	slice, or map and T1 is executed. If the value is a map and the
	keys are of basic type with a defined order ("comparable"), the
	elements will be visited in sorted key order.

https://codereview.appspot.com/7341053/diff/1001/lib/godoc/package.html#newco...
lib/godoc/package.html:169: <h2 id="pkg-{{$marker}}">{{noteTitle $marker |
html}}s</h2>
On 2013/02/20 06:05:41, gburd wrote:
> In browsers with case-insensitive element ids, this id can clash with other
ids
> in the document. Consider using <h2 id="pkg-note-{{$marker}}">.

Done.

[cnicolaou, 2013-02-20 19:50:11]

Hello gri@golang.org, gri@google.com, gary.burd@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[gri1, 2013-02-20 19:55:40]

On Tue, Feb 19, 2013 at 10:05 PM, <gary.burd@gmail.com> wrote:

> The Bugs field must be populated to honor the compatibility guarantee
> that programs will continue to run correctly.
>

Gary, can you elaborate a bit more on this? What programs require a
non-empty BUG fields or otherwise they will break? Or more specifically,
why is it crucial that there is no transition phase (from the old to the
new way) where bugs may not be shown? Thanks.

- gri

[gri, 2013-02-20 20:04:26]

I think this is mostly good, but let's wait for Gary's response.
Thanks.
- gri

https://codereview.appspot.com/7341053/diff/8001/src/pkg/go/doc/doc.go
File src/pkg/go/doc/doc.go (right):

https://codereview.appspot.com/7341053/diff/8001/src/pkg/go/doc/doc.go#newcode20
src/pkg/go/doc/doc.go:20: // DEPRECATED, this is now empty and BUGS appear in
Notes below.
Bugs is deprecated, but not empty with the recent change. But let's see what
Gary Burd says before moving on.

[gburd, 2013-02-20 22:33:04]

> What programs require a non-empty BUG fields or otherwise
> they will break? Or more specifically, why is it crucial that there
> is no transition phase (from the old to the new way) where bugs
> may not be shown?.

Any program that displays bugs using the Bugs field is broken by the first delta
in this change set. The program will compile and run, but not run correctly. The
Go 1 compatibilty guaratee does not allow for a transition phase where programs
are broken until updated to the new way of doing things.

[dsymonds, 2013-02-20 23:12:08]

How about making the new Notes field be defined as containing all
non-BUG notes. Then the Bugs field remains as is, existing tools work
fine, and you just have to consult two fields to get the full list.

[cnicolaou, 2013-02-20 23:38:29]

On 2013/02/20 22:33:04, gburd wrote:
> > What programs require a non-empty BUG fields or otherwise
> > they will break? Or more specifically, why is it crucial that there
> > is no transition phase (from the old to the new way) where bugs
> > may not be shown?.
> 
> Any program that displays bugs using the Bugs field is broken by the first
delta
> in this change set. The program will compile and run, but not run correctly.
The
> Go 1 compatibilty guaratee does not allow for a transition phase where
programs
> are broken until updated to the new way of doing things.

sure, I am now populating that field as before, so nothing should break.

[cnicolaou, 2013-02-20 23:39:32]

On 2013/02/20 23:12:08, dsymonds wrote:
> How about making the new Notes field be defined as containing all
> non-BUG notes. Then the Bugs field remains as is, existing tools work
> fine, and you just have to consult two fields to get the full list.

it's tedious to code for this since templates have to use both. If you use
.Notes you shouldn't have to use .Bugs at all. The current change allows for
that by populate .Bugs as well as putting it in .Notes.

[gri, 2013-02-21 20:53:42]

This looks good overall now; and I've verified that it works and doesn't disturb
the "old" godoc output w/o a -notes flag.

A couple of things:

- I made some significant changes to godoc and you'll need to merge against them
first and re-upload (my apologies)

- Please simplify the notes extraction below.

For a future CL:

The extraction of notes needs more work, eventually. Sometimes we have multiple
TODOs on adjacent lines and the extraction mechanism just picks the first one
and glues everything else together. The right approach may be to fix the source
code (note comments must be separated from other note comments w/ code or empty
lines).

https://codereview.appspot.com/7341053/diff/8001/src/cmd/godoc/godoc.go
File src/cmd/godoc/godoc.go (right):

https://codereview.appspot.com/7341053/diff/8001/src/cmd/godoc/godoc.go#newco...
src/cmd/godoc/godoc.go:1122: noteMap := make(map[string]bool)
I propose to do this more directly: instead of removing notes from pdoc.Notes
(and thereby writing a data structure that godoc doesn't necessarily "own"), why
not simply iterate through the splitted string and pick the relevant notes?
Something along the lines of:

notes := make(map[string][]string)
for _, key := range strings.Split(*notes, ",") {
    if n := pdoc.Notes[key]; n != nil {
        notes[key] = n
    }
}

and then store notes in an new field Notes of PageInfo. The string splitting
also really needs to be done only once (upon startup of godoc). I'm fine with
doing it each time, or having a global variable that is initialized accordingly.

[rsc, 2013-02-21 20:56:10]

I haven't looked closely, but I see a lot of string-keyed maps floating
around. Remember that iterating over a map in a template will produce a
different order each time the template runs.

Russ

[gri1, 2013-02-21 21:24:03]

Sigh, yes, I glanced over this. We need to jave sorted data structures
for reproducible test cases. I'll have another look at this, later
today.
- gri

On Thu, Feb 21, 2013 at 12:56 PM, Russ Cox <rsc@golang.org> wrote:
> I haven't looked closely, but I see a lot of string-keyed maps floating
> around. Remember that iterating over a map in a template will produce a
> different order each time the template runs.
>
> Russ
>

[cnicolaou, 2013-02-24 21:40:15]

I've made the changes you asked for above. I'm using PageInfo.Notes for the top
level Notes that contains the extracted Notes, but this is easy to get wrong in
the templates (e.g. $.Notes vs .Notes inside a with statement). Maybe I should
use PageInfo.NotesToShow or some such? Let me know before I push the updated CL.


We agreed offline that templates sort map keys, so that's all good.

Cheers, Cos.


On 2013/02/21 21:24:03, gri1 wrote:
> Sigh, yes, I glanced over this. We need to jave sorted data structures
> for reproducible test cases. I'll have another look at this, later
> today.
> - gri
> 
> On Thu, Feb 21, 2013 at 12:56 PM, Russ Cox <mailto:rsc@golang.org> wrote:
> > I haven't looked closely, but I see a lot of string-keyed maps floating
> > around. Remember that iterating over a map in a template will produce a
> > different order each time the template runs.
> >
> > Russ
> >

[kevlar, 2013-02-24 21:55:28]

text/template sorts its map keys:
"If the value is a map and the


	keys are of basic type with a defined order ("comparable"), the
	elements will be visited in sorted key order.



On Thu, Feb 21, 2013 at 12:56 PM, Russ Cox <rsc@golang.org> wrote:

> I haven't looked closely, but I see a lot of string-keyed maps floating
> around. Remember that iterating over a map in a template will produce a
> different order each time the template runs.
>
> Russ
>
>  --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>
>

[cnicolaou, 2013-02-25 20:58:37]

Hello gri@golang.org, gri@google.com, gary.burd@gmail.com, dsymonds@golang.org,
rsc@golang.org, kevlar@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[gri, 2013-02-26 00:31:07]

LGTM

Can you please re-sync and re-upload? I get

abort: codereview issue 7341053 is out of date: patch and recent changes
conflict (083759101bc9->ebc229da2df9)

- gri

[cnicolaou, 2013-02-26 00:55:19]

Hello gri@golang.org, gri@google.com, gary.burd@gmail.com, dsymonds@golang.org,
rsc@golang.org, kevlar@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[gri, 2013-02-26 04:29:59]

LGTM

thanks.

PS: There appears to be an extra newline between the last documented entry and
the Bugs section in the command line mode. E.g. compare the output of command
"godoc bytes" before and after. That said, the text template is notoriously
subtle to get right w/ respect to spacing (e.g. functions have 2 empty lines
between them, not due to this CL, I don't know when that happened). Anyway, will
submit now. We can fine-tune later.

[gri, 2013-02-26 04:34:57]

*** Submitted as https://code.google.com/p/go/source/detail?r=7988a4263b4b ***

cmd/godoc: add support for display Notes parsed by pkg/go/doc
pkg/go/doc: move BUG notes from Package.Bugs to the general Package.Notes field.
Removing .Bugs would break existing code so it's left in for now.

R=gri, gri, gary.burd, dsymonds, rsc, kevlar
CC=golang-dev
https://codereview.appspot.com/7341053

Committer: Robert Griesemer <gri@golang.org>

[minux1, 2013-02-28 19:14:51]

This seems to display any Notes found not only at top (package/file) level
but also within functions.

try this:
$ GOOS=darwin GOARCH=amd64 godoc syscall None
PACKAGE
package syscall
    import "syscall"
NOTE <snip>
TODO
    pass through to C


The final TODO note is very confusing.
we'd better attach some context info to it, or remove it from the source,
or only show package/file level Notices and trust the author to provide
sufficient context information.

FYI: that todo is in func Clearenv() in pkg/syscall/env_unix.go.

[gri1, 2013-02-28 19:17:30]

I believe this is no different from Bugs. That said, we know that we need
to better format the output, and do a better job with collecting the right
text for TODOs and other notes.

In general, we probably don't want to show TODOs in docs - they tend to be
internal things.

Also, one is free to choose a different tag for external TODOs if they
should be shown in the docs.

- gri


On Thu, Feb 28, 2013 at 11:14 AM, <minux.ma@gmail.com> wrote:

> This seems to display any Notes found not only at top (package/file)
> level
> but also within functions.
>
> try this:
> $ GOOS=darwin GOARCH=amd64 godoc syscall None
> PACKAGE
> package syscall
>     import "syscall"
> NOTE <snip>
> TODO
>     pass through to C
>
>
> The final TODO note is very confusing.
> we'd better attach some context info to it, or remove it from the
> source,
> or only show package/file level Notices and trust the author to provide
> sufficient context information.
>
> FYI: that todo is in func Clearenv() in pkg/syscall/env_unix.go.
>
>
https://codereview.appspot.**com/7341053/<https://codereview.appspot.com/7341...
>

[minux1, 2013-02-28 19:26:28]

On Fri, Mar 1, 2013 at 3:17 AM, Robert Griesemer <gri@google.com> wrote:

> I believe this is no different from Bugs. That said, we know that we need
> to better format the output, and do a better job with collecting the right
> text for TODOs and other notes.
>
agreed.

>
> In general, we probably don't want to show TODOs in docs - they tend to be
> internal things.
>
however, sometimes one do use TODO to document limitations of a package.
i propose we only display package level NOTEs in docs (by default), or we
can
arrange function level NOTEs to be displayed only as docs for that function.

[cnicolaou, 2013-02-28 23:05:02]

On Thu, Feb 28, 2013 at 11:26 AM, minux <minux.ma@gmail.com> wrote:

>
> On Fri, Mar 1, 2013 at 3:17 AM, Robert Griesemer <gri@google.com> wrote:
>
>> I believe this is no different from Bugs. That said, we know that we need
>> to better format the output, and do a better job with collecting the right
>> text for TODOs and other notes.
>>
> agreed.
>

yep, for sure, both for the original BUGs and arbitrary notes.

>
>
>>
>> In general, we probably don't want to show TODOs in docs - they tend to
>> be internal things.
>>
> however, sometimes one do use TODO to document limitations of a package.
> i propose we only display package level NOTEs in docs (by default), or we
> can
> arrange function level NOTEs to be displayed only as docs for that
> function.
>

If a given team/project wants to do this, they can simply agree to use one
marker for public comments and another for internal ones and then just use
godoc accordingly. E.g. TODO for top level ones and todo() for internal
ones - the second lower case one won't get detected anyway. Isn't this good
enough to achieve the desired effect - it certainly meets my goals which
led to this change originally.

Cheers, Cos.