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

[g...@golang.org]

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

https://codereview.appspot.com/7341053/

[cnic...@google.com]

Hello g...@golang.org, g...@google.com, gary...@gmail.com,
dsym...@golang.org, r...@golang.org, kev...@google.com (cc:
golan...@googlegroups.com),

Please take another look.


https://codereview.appspot.com/7341053/

[g...@golang.org]

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.

https://codereview.appspot.com/7341053/

[g...@golang.org]

*** 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 <g...@golang.org>


https://codereview.appspot.com/7341053/

[minu...@gmail.com]

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/

[Robert Griesemer]

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

[minux]

On Fri, Mar 1, 2013 at 3:17 AM, Robert Griesemer <g...@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.

[Cosmos Nicolaou]

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

On Fri, Mar 1, 2013 at 3:17 AM, Robert Griesemer <g...@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.