code review 7496048: go/doc, godoc: improved note reading

go/doc, godoc: improved note reading

- A note doesn't have to be in the first
comment of a comment group anymore, and
several notes may appear in the same comment
group (e.g., it is fairly common to have a
TODO(uid) note immediately following another
comment).

- Define a doc.Note type which also contains
note uid and position info.

- Better formatting in godoc output. The position
information is not yet used, but could be used to
locate the note in the source text if desired.

Fixes issue 4843.

[gri, 2013-03-15 00:00:40]

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

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

[gobot, 2013-03-15 05:56:10]

R=r (assigned by dsymonds)

[cnicolaou, 2013-03-15 18:06:21]

On 2013/03/15 00:00:40, gri wrote:
> Hello mailto:cnicolaou@google.com (cc: mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go

I think godoc text and html output needs some tweaking for multi-line comments. 

// TODO(cos): foo
//      - bar
//      - baz
//
// TODO(cos): on its own
// TODO(cos): gets rolled into the previous one
//

produces:

TODOS

    foo

	- bar
	- baz

    TODO(cos): on its own TODO(cos): gets rolled into the previous one

[cnicolaou, 2013-03-15 18:06:30]

https://codereview.appspot.com/7496048/diff/13001/src/pkg/go/doc/testdata/a0.go
File src/pkg/go/doc/testdata/a0.go (right):

https://codereview.appspot.com/7496048/diff/13001/src/pkg/go/doc/testdata/a0....
src/pkg/go/doc/testdata/a0.go:30: // - this is the last line of note 4
how about a test case without the : since it's now optional?

[gri, 2013-03-15 23:32:17]

PTAL

fwiw, I cannot reproduce the case where two cases get rolled together into a
single one. For one, the go/doc test case explicitly test this (and separates
the notes), and godoc doesn't combine them again.

Perhaps you ran a godoc that was not linked against the patched version of
go/doc?

- gri

https://codereview.appspot.com/7496048/diff/13001/src/pkg/go/doc/testdata/a0.go
File src/pkg/go/doc/testdata/a0.go (right):

https://codereview.appspot.com/7496048/diff/13001/src/pkg/go/doc/testdata/a0....
src/pkg/go/doc/testdata/a0.go:30: // - this is the last line of note 4
On 2013/03/15 18:06:31, cnicolaou wrote:
> how about a test case without the : since it's now optional?

Done.

[cnicolaou, 2013-03-19 05:14:29]

On 2013/03/15 23:32:17, gri wrote:
> PTAL
> 
> fwiw, I cannot reproduce the case where two cases get rolled together into a
> single one. For one, the go/doc test case explicitly test this (and separates
> the notes), and godoc doesn't combine them again.
> 
> Perhaps you ran a godoc that was not linked against the patched version of
> go/doc?

I thought I had, but it seems that clpatch had failed and I didn't notice:-(
Merging multiple notes into one line doesn't happen, but multi-line notes get
merged into line. Is this what you want, or would you rather have a synopsis
like function as per the test example?

Probably for a separate CL, but how about generating an anchor for the notes and
a link to them from the docs so that clicking on a note in the html output takes
you the location in the source code.

Cheers, Cos.

> 
> - gri
> 
>
https://codereview.appspot.com/7496048/diff/13001/src/pkg/go/doc/testdata/a0.go
> File src/pkg/go/doc/testdata/a0.go (right):
> 
>
https://codereview.appspot.com/7496048/diff/13001/src/pkg/go/doc/testdata/a0....
> src/pkg/go/doc/testdata/a0.go:30: // - this is the last line of note 4
> On 2013/03/15 18:06:31, cnicolaou wrote:
> > how about a test case without the : since it's now optional?
> 
> Done.

[gri, 2013-03-19 05:29:38]

On Mon, Mar 18, 2013 at 10:14 PM, <cnicolaou@google.com> wrote:

> On 2013/03/15 23:32:17, gri wrote:
>
>> PTAL
>>
>
>  fwiw, I cannot reproduce the case where two cases get rolled together
>>
> into a
>
>> single one. For one, the go/doc test case explicitly test this (and
>>
> separates
>
>> the notes), and godoc doesn't combine them again.
>>
>
>  Perhaps you ran a godoc that was not linked against the patched
>>
> version of
>
>> go/doc?
>>
>
> I thought I had, but it seems that clpatch had failed and I didn't
> notice:-( Merging multiple notes into one line doesn't happen, but
> multi-line notes get merged into line. Is this what you want, or would
> you rather have a synopsis like function as per the test example?
>

I don't know what the best approach is here. The previous solution simply
replicated the TODO text, line breaks, indentation and all. That's mostly
ok in html (it all disappears), but looks odd in the text version. I've
made the - clearly - crude assumption that TODO's tend to be short, and if
they are longer, they are essentially prose explaining the TODO. Thus, it
should be ok to simply flatten it out. One could have a synopsis-like
shortcut, that would be easy.

>
> Probably for a separate CL, but how about generating an anchor for the
> notes and a link to them from the docs so that clicking on a note in the
> html output takes you the location in the source code.
>

Yes, the anchor would be useful. I didn't put it in because I think the
existing template functions aren't immediately useable and I may have to
add some more code there. Fine to do in a follow up CL.

>
> Cheers, Cos.
>
>
>
>  - gri
>>
>
>
> https://codereview.appspot.**com/7496048/diff/13001/src/**
>
pkg/go/doc/testdata/a0.go<https://codereview.appspot.com/7496048/diff/13001/src/pkg/go/doc/testdata/a0.go>
>
>> File src/pkg/go/doc/testdata/a0.go (right):
>>
>
>
> https://codereview.appspot.**com/7496048/diff/13001/src/**
>
pkg/go/doc/testdata/a0.go#**newcode30<https://codereview.appspot.com/7496048/diff/13001/src/pkg/go/doc/testdata/a0.go#newcode30>
>
>> src/pkg/go/doc/testdata/a0.go:**30: // - this is the last line of note 4
>> On 2013/03/15 18:06:31, cnicolaou wrote:
>> > how about a test case without the : since it's now optional?
>>
>
>  Done.
>>
>
>
>
>
https://codereview.appspot.**com/7496048/<https://codereview.appspot.com/7496...
>

[cnicolaou, 2013-03-19 14:41:23]

On 2013/03/19 05:29:38, gri wrote:
> On Mon, Mar 18, 2013 at 10:14 PM, <mailto:cnicolaou@google.com> wrote:
> 
> > On 2013/03/15 23:32:17, gri wrote:
> >
> >> PTAL
> >>
> >
> >  fwiw, I cannot reproduce the case where two cases get rolled together
> >>
> > into a
> >
> >> single one. For one, the go/doc test case explicitly test this (and
> >>
> > separates
> >
> >> the notes), and godoc doesn't combine them again.
> >>
> >
> >  Perhaps you ran a godoc that was not linked against the patched
> >>
> > version of
> >
> >> go/doc?
> >>
> >
> > I thought I had, but it seems that clpatch had failed and I didn't
> > notice:-( Merging multiple notes into one line doesn't happen, but
> > multi-line notes get merged into line. Is this what you want, or would
> > you rather have a synopsis like function as per the test example?
> >
> 
> I don't know what the best approach is here. The previous solution simply
> replicated the TODO text, line breaks, indentation and all. That's mostly
> ok in html (it all disappears), but looks odd in the text version. I've
> made the - clearly - crude assumption that TODO's tend to be short, and if
> they are longer, they are essentially prose explaining the TODO. Thus, it
> should be ok to simply flatten it out. One could have a synopsis-like
> shortcut, that would be easy.
> 
> >
> > Probably for a separate CL, but how about generating an anchor for the
> > notes and a link to them from the docs so that clicking on a note in the
> > html output takes you the location in the source code.
> >
> 
> Yes, the anchor would be useful. I didn't put it in because I think the
> existing template functions aren't immediately useable and I may have to
> add some more code there. Fine to do in a follow up CL.

ok, sounds reasonable. Maybe the synopsis approach will look nicer as the anchor
text, so let's just defer that to then. I'm happy to approve but I don't see
where to do so in this UI?

Cheers, Cos.
> 
> >
> > Cheers, Cos.
> >
> >
> >
> >  - gri
> >>
> >
> >
> > https://codereview.appspot.**com/7496048/diff/13001/src/**
> >
>
pkg/go/doc/testdata/a0.go<https://codereview.appspot.com/7496048/diff/13001/src/pkg/go/doc/testdata/a0.go>
> >
> >> File src/pkg/go/doc/testdata/a0.go (right):
> >>
> >
> >
> > https://codereview.appspot.**com/7496048/diff/13001/src/**
> >
>
pkg/go/doc/testdata/a0.go#**newcode30<https://codereview.appspot.com/7496048/diff/13001/src/pkg/go/doc/testdata/a0.go#newcode30>
> >
> >> src/pkg/go/doc/testdata/a0.go:**30: // - this is the last line of note 4
> >> On 2013/03/15 18:06:31, cnicolaou wrote:
> >> > how about a test case without the : since it's now optional?
> >>
> >
> >  Done.
> >>
> >
> >
> >
> >
>
https://codereview.appspot.**com/7496048/%3Chttps://codereview.appspot.com/74...>
> >

[cnicolaou, 2013-03-19 17:10:21]

lgtm.

[gri, 2013-03-19 18:15:10]

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

go/doc, godoc: improved note reading

- A note doesn't have to be in the first
comment of a comment group anymore, and
several notes may appear in the same comment
group (e.g., it is fairly common to have a
TODO(uid) note immediately following another
comment).

- Define a doc.Note type which also contains
note uid and position info.

- Better formatting in godoc output. The position
information is not yet used, but could be used to
locate the note in the source text if desired.

Fixes issue 4843.

R=r, cnicolaou
CC=gobot, golang-dev
https://codereview.appspot.com/7496048