code review 7322061: go/doc: add support for arbitrary notes

go/doc: add support for arbitrary notes

Add support for arbitrary notes of the form // MARKER(userid): comment 
in the same vein as BUG(userid): A marker must be two or more upper case [A-Z] letters.

[cnicolaou, 2013-02-13 23:31:42]

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

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

[rsc, 2013-02-14 02:15:51]

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

https://codereview.appspot.com/7322061/diff/3/src/pkg/go/doc/doc.go#newcode21
src/pkg/go/doc/doc.go:21: // Additional annotations such as TODO(userid): or
SECURITY(userid):
How about

Notes map[string][]string

and just record anything matching [A-Z][A-Z]+ at top level, instead of adding
the var Annotations below.

[gri, 2013-02-14 02:46:17]

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

https://codereview.appspot.com/7322061/diff/3/src/pkg/go/doc/doc.go#newcode21
src/pkg/go/doc/doc.go:21: // Additional annotations such as TODO(userid): or
SECURITY(userid):
On 2013/02/14 02:15:51, rsc wrote:
> How about
> 
> Notes map[string][]string
> 
> and just record anything matching [A-Z][A-Z]+ at top level, instead of adding
> the var Annotations below.

Yep. I suggested the map at lunch, today, and the list of "keywords"
(Annotations) to look for. But just recording everything is even simpler.

https://codereview.appspot.com/7322061/diff/3/src/pkg/go/doc/reader.go
File src/pkg/go/doc/reader.go (right):

https://codereview.appspot.com/7322061/diff/3/src/pkg/go/doc/reader.go#newcod...
src/pkg/go/doc/reader.go:406: annotation_regexp_format = "^/[/*][
\t]*(BUG%s)\\(.*\\):[ \t]*(.*)" // BUG(uid): or BUG|ANOTHERMARKER(uid):
per Russ' suggestion just look for all

// MARKER(uid): xxx

where marker is [A-Z]+

https://codereview.appspot.com/7322061/diff/3/src/pkg/go/doc/reader.go#newcod...
src/pkg/go/doc/reader.go:496: if marker, text, present := readAnnotation(c);
present {
This should become simpler - likely close to what there was originally.

[cnicolaou, 2013-02-14 18:28:29]

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

Please take another look.

[bradfitz, 2013-02-14 18:44:58]

https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go
File src/pkg/go/doc/reader.go (right):

https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go#new...
src/pkg/go/doc/reader.go:404: annotation_marker  = regexp.MustCompile("^/[/*][
\t]*([A-Z][A-Z]+)\\(.*\\):[ \t]*(.*)") // MARKER(uid)
this must be ancient code because we no longer use underscores in variable
names.  annotationMarker.

also, use backticks `` so you can remove the double backslashes like \\(.

and shouldn't the username in parens be at least one character? \(.+\)?  I've
never seen BUG(): foo.  But I have seen TODO: foo.

https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go#new...
src/pkg/go/doc/reader.go:405: annotation_content = regexp.MustCompile("[^
\n\r\t]+")                                  // at least one non-whitespace char
annotationContent and backticks.

[bradfitz, 2013-02-14 18:45:36]

Also, CL description should be of the form:

cmd/godoc: add support for arbitrary annotations

(blank line, then arbitrary lengthy description)

[gri, 2013-02-14 18:47:29]

https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go
File src/pkg/go/doc/reader.go (right):

https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go#new...
src/pkg/go/doc/reader.go:408: func readAnnotation(c *ast.CommentGroup) (marker,
annotation string, found bool) {
just return marker, annotation and use marker != "" as "found" indicator

https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go#new...
src/pkg/go/doc/reader.go:495: if r.annotations[marker] == nil {
this if should not be needed: append works fine with a nil list as long as it's
typed, which it is. That is:

r.annotations[marker] = append(r.annotations[marker], text)

is sufficient

https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go#new...
src/pkg/go/doc/reader.go:500: continue
remove continue

[rsc, 2013-02-14 18:54:29]

(And in response to Brad, yes, this is ancient code.)

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

https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/doc.go#newcode24
src/pkg/go/doc/doc.go:24: // BUGS is explicitly not included in these
annotations but will
s/BUGS/BUG/

https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/doc.go#newcode26
src/pkg/go/doc/doc.go:26: Annotations map[string][]string
s/Annotations/Notes/
just as clear, far easier to type

https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go
File src/pkg/go/doc/reader.go (right):

https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go#new...
src/pkg/go/doc/reader.go:152: annotations map[string][]string
notes

[jscrockett01, 2013-02-14 19:01:27]

Has it been considered to allow post-^ whitespace for annotations inside 
functions or at the end of lines?  Such as:

x++ // TODO: there should be a better way




On Thursday, February 14, 2013 1:44:58 PM UTC-5, Brad Fitzpatrick wrote:
>
>
> https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go 
> File src/pkg/go/doc/reader.go (right): 
>
>
https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go#new...

>
>
src/pkg/go/doc/reader.go:404<https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go#newcode404src/pkg/go/doc/reader.go:404>:

> annotation_marker  = 
> regexp.MustCompile("^/[/*][ \t]*([A-Z][A-Z]+)\\(.*\\):[ \t]*(.*)") // 
> MARKER(uid) 
> this must be ancient code because we no longer use underscores in 
> variable names.  annotationMarker. 
>
> also, use backticks `` so you can remove the double backslashes like 
> \\(. 
>
> and shouldn't the username in parens be at least one character? \(.+\)? 
> I've never seen BUG(): foo.  But I have seen TODO: foo. 
>
>
https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go#new...

>
>
src/pkg/go/doc/reader.go:405<https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go#newcode405src/pkg/go/doc/reader.go:405>:

> annotation_content = 
> regexp.MustCompile("[^ \n\r\t]+")                                  // at 
> least one non-whitespace char 
> annotationContent and backticks. 
>
> https://codereview.appspot.com/7322061/ 
>

[cnicolaou, 2013-02-14 19:43:57]

On 2013/02/14 18:44:58, bradfitz wrote:
> https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go
> File src/pkg/go/doc/reader.go (right):
> 
>
https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go#new...
> src/pkg/go/doc/reader.go:404: annotation_marker  = regexp.MustCompile("^/[/*][
> \t]*([A-Z][A-Z]+)\\(.*\\):[ \t]*(.*)") // MARKER(uid)
> this must be ancient code because we no longer use underscores in variable
> names.  annotationMarker.
> 
> also, use backticks `` so you can remove the double backslashes like \\(.
> 
> and shouldn't the username in parens be at least one character? \(.+\)?  I've
> never seen BUG(): foo.  But I have seen TODO: foo.
> 
>
https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go#new...
> src/pkg/go/doc/reader.go:405: annotation_content = regexp.MustCompile("[^
> \n\r\t]+")                                  // at least one non-whitespace
char
> annotationContent and backticks.

done. I agree about needing a userid of at least one char, so I've made that
regexp change.  A quick grep over all the core code doesn't show up any BUG():
instances without a userid. I think it's generally good practice to insist on a
userid being there regardless of the note. Thanks.

[cnicolaou, 2013-02-14 19:44:16]

On 2013/02/14 18:45:36, bradfitz wrote:
> Also, CL description should be of the form:
> 
> cmd/godoc: add support for arbitrary annotations
> 
> (blank line, then arbitrary lengthy description)

done.

[cnicolaou, 2013-02-14 19:44:27]

On 2013/02/14 18:47:29, gri wrote:
> https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go
> File src/pkg/go/doc/reader.go (right):
> 
>
https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go#new...
> src/pkg/go/doc/reader.go:408: func readAnnotation(c *ast.CommentGroup)
(marker,
> annotation string, found bool) {
> just return marker, annotation and use marker != "" as "found" indicator
> 
>
https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go#new...
> src/pkg/go/doc/reader.go:495: if r.annotations[marker] == nil {
> this if should not be needed: append works fine with a nil list as long as
it's
> typed, which it is. That is:
> 
> r.annotations[marker] = append(r.annotations[marker], text)
> 
> is sufficient
> 
>
https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go#new...
> src/pkg/go/doc/reader.go:500: continue
> remove continue

done.

[cnicolaou, 2013-02-14 19:44:47]

On 2013/02/14 18:54:29, rsc wrote:
> (And in response to Brad, yes, this is ancient code.)
> 
> https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/doc.go
> File src/pkg/go/doc/doc.go (right):
> 
>
https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/doc.go#newcode24
> src/pkg/go/doc/doc.go:24: // BUGS is explicitly not included in these
> annotations but will
> s/BUGS/BUG/
> 
>
https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/doc.go#newcode26
> src/pkg/go/doc/doc.go:26: Annotations map[string][]string
> s/Annotations/Notes/
> just as clear, far easier to type
> 
> https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go
> File src/pkg/go/doc/reader.go (right):
> 
>
https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go#new...
> src/pkg/go/doc/reader.go:152: annotations map[string][]string
> notes

done.

[cnicolaou, 2013-02-14 19:47:11]

On 2013/02/14 19:01:27, jscrockett01 wrote:
> Has it been considered to allow post-^ whitespace for annotations inside 
> functions or at the end of lines?  Such as:
> 
> x++ // TODO: there should be a better way

I wasn't envisaging catching every last TODO that we put in our code, just the
ones that we consider worth putting in the documentation and hence for my usage
it seems ok to have them standalone.

> 
> 
> 
> 
> On Thursday, February 14, 2013 1:44:58 PM UTC-5, Brad Fitzpatrick wrote:
> >
> >
> > https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go 
> > File src/pkg/go/doc/reader.go (right): 
> >
> >
>
https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go#new...
> 
> >
> >
>
src/pkg/go/doc/reader.go:404<https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go#newcode404src/pkg/go/doc/reader.go:404>:
> 
> > annotation_marker  = 
> > regexp.MustCompile("^/[/*][ \t]*([A-Z][A-Z]+)\\(.*\\):[ \t]*(.*)") // 
> > MARKER(uid) 
> > this must be ancient code because we no longer use underscores in 
> > variable names.  annotationMarker. 
> >
> > also, use backticks `` so you can remove the double backslashes like 
> > \\(. 
> >
> > and shouldn't the username in parens be at least one character? \(.+\)? 
> > I've never seen BUG(): foo.  But I have seen TODO: foo. 
> >
> >
>
https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go#new...
> 
> >
> >
>
src/pkg/go/doc/reader.go:405<https://codereview.appspot.com/7322061/diff/9001/src/pkg/go/doc/reader.go#newcode405src/pkg/go/doc/reader.go:405>:
> 
> > annotation_content = 
> > regexp.MustCompile("[^ \n\r\t]+")                                  // at 
> > least one non-whitespace char 
> > annotationContent and backticks. 
> >
> > https://codereview.appspot.com/7322061/ 
> >

[cnicolaou, 2013-02-14 19:47:31]

Hello gri@golang.org, rsc@golang.org, bradfitz@golang.org,
jscrockett01@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[gri, 2013-02-14 21:00:21]

LGTM

https://codereview.appspot.com/7322061/diff/10009/src/pkg/go/doc/reader.go
File src/pkg/go/doc/reader.go (right):

https://codereview.appspot.com/7322061/diff/10009/src/pkg/go/doc/reader.go#ne...
src/pkg/go/doc/reader.go:410: if m := noteMarker.FindStringSubmatch(text); m !=
nil {
Maybe i'm sleep-deprived, but can't we just use a singular regexp here that uses
the noteContent expr in noteMarker rather than (.*) ? If it doesn't match, we're
done. (I know the old code was doing the same as you're doing now).

[cnicolaou, 2013-02-14 22:44:19]

On 2013/02/14 21:00:21, gri wrote:
> LGTM
> 
> https://codereview.appspot.com/7322061/diff/10009/src/pkg/go/doc/reader.go
> File src/pkg/go/doc/reader.go (right):
> 
>
https://codereview.appspot.com/7322061/diff/10009/src/pkg/go/doc/reader.go#ne...
> src/pkg/go/doc/reader.go:410: if m := noteMarker.FindStringSubmatch(text); m
!=
> nil {
> Maybe i'm sleep-deprived, but can't we just use a singular regexp here that
uses
> the noteContent expr in noteMarker rather than (.*) ? If it doesn't match,
we're
> done. (I know the old code was doing the same as you're doing now).

well, I tried it to be sure, but the two are a little different. The current
form allows for multiword comments, i.e. with spaces in them,  provided there is
at least one non-whitespace character there. The change to replace the .* with
[^ \n\r\t]+ will only extract the first word from the comment, removing the
space leaves you open to only whitespace comments. So I think the current form
is what you want. I can well believe that there is a regex that does exactly
what we want, but it's not obvious to me that it would be simpler than what we
have now.

[gri, 2013-02-15 04:21:08]

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

go/doc: add support for arbitrary notes

Add support for arbitrary notes of the form // MARKER(userid): comment
in the same vein as BUG(userid): A marker must be two or more upper case [A-Z]
letters.

R=gri, rsc, bradfitz, jscrockett01
CC=golang-dev
https://codereview.appspot.com/7322061

Committer: Robert Griesemer <gri@golang.org>