code review 5437056: go/doc: Detect headings in comments and format them as ...

go/doc: Detect headings in comments and format them as h3 in html.

To structure larger sections of comments in html output headings
are detected in comments and formated as h3 in the generated html.
A simple heuristic is used to detect headings in comments:
A heading is a non-blank, non-indented line preceded by a blank
line. It is followed by a blank and a non-blank, non-indented line.
A heading must start with an uppercase letter and end with a letter,
digit or a colon.  A heading may not contain punctuation characters.

[volker.dobler, 2011-11-24 11:57:26]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[jnml, 2011-11-24 18:13:14]

On 2011/11/24 11:57:26, volker.dobler wrote:
> I'd like you to review this change to

Excerpts from the discussion[1] (predating this CL) about the idea:

----
Andrew Gerrand, Nov 23 (1 day ago)

We have deliberately avoided creating some kind of markup language for godoc
comments.
----
r, Nov 23 (1 day ago)

I'd rather not complicate the formatting any more. Automatic detection of
hyperlinks is about as far as I'd go.
----
r, Nov 23 (1 day ago)

I speak from long experience when I say that if you give people formatting
tools, they will concentrate on the formatting rather than the quality of their
prose.
----

[1] https://groups.google.com/d/topic/golang-nuts/iC77ywOFpe0/discussion

[gri, 2011-11-25 20:41:11]

Some comments. But I like to think about this some more before we are going
ahead with it.
- gri

http://codereview.appspot.com/5437056/diff/5001/src/pkg/go/doc/comment.go
File src/pkg/go/doc/comment.go (right):

http://codereview.appspot.com/5437056/diff/5001/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:282: switch s[len(s)-2] {
The function needs a comment stating that s is at least 2 bytes long. Better
would be to just be safe and check for it (this is unlikely to have an impact on
overall performance).

if len(s) < 2 {
   return false
}
// len(s) >= 2

at the beginning of the function body

http://codereview.appspot.com/5437056/diff/5001/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:296: if c == 32 {
s/32/' '/

what about tabs? should have at least a comment

http://codereview.appspot.com/5437056/diff/5001/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:297: if n == 4 {
make 5 a constant and use an expression here - makes the connection between 5
and 4 clear

http://codereview.appspot.com/5437056/diff/5001/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:377: if blanks == 2 && len(lines) > i+2 &&
isBlank(lines[i+1]) && !isBlank(lines[i+2]) {
section could use a comment (just before if statement)

[volker.dobler, 2011-11-28 00:01:51]

Hello golang-dev@googlegroups.com, jan.mercl@nic.cz, gri@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[volker.dobler, 2011-11-28 00:04:30]

http://codereview.appspot.com/5437056/diff/5001/src/pkg/go/doc/comment.go
File src/pkg/go/doc/comment.go (right):

http://codereview.appspot.com/5437056/diff/5001/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:282: switch s[len(s)-2] {
On 2011/11/25 20:41:11, gri wrote:
> The function needs a comment stating that s is at least 2 bytes long. Better
> would be to just be safe and check for it (this is unlikely to have an impact
on
> overall performance).
> 
> if len(s) < 2 {
>    return false
> }
> // len(s) >= 2
> 
> at the beginning of the function body

Done.

http://codereview.appspot.com/5437056/diff/5001/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:296: if c == 32 {
On 2011/11/25 20:41:11, gri wrote:
> s/32/' '/
> 
> what about tabs? should have at least a comment

Done. Tab treated as space.

http://codereview.appspot.com/5437056/diff/5001/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:297: if n == 4 {
On 2011/11/25 20:41:11, gri wrote:
> make 5 a constant and use an expression here - makes the connection between 5
> and 4 clear

Done.

http://codereview.appspot.com/5437056/diff/5001/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:377: if blanks == 2 && len(lines) > i+2 &&
isBlank(lines[i+1]) && !isBlank(lines[i+2]) {
On 2011/11/25 20:41:11, gri wrote:
> section could use a comment (just before if statement)

Done.

[adg, 2011-11-28 05:33:44]

http://codereview.appspot.com/5437056/diff/3005/src/pkg/go/doc/comment.go
File src/pkg/go/doc/comment.go (right):

http://codereview.appspot.com/5437056/diff/3005/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:275: const maxWordsInHeading = 5 // a heading has at
most that many words
I don't think we need this heuristic. I can't think of a correctly-formatted
single line that the other heuristics would not catch.

http://codereview.appspot.com/5437056/diff/3005/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:283: func isHeading(s []byte) bool {
s/s/line/
to make it clear what the function expects

http://codereview.appspot.com/5437056/diff/3005/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:287: // len(s) >= 2
drop this comment

http://codereview.appspot.com/5437056/diff/3005/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:322: // Enclose headings into <h3> tags.
s/into/with/

http://codereview.appspot.com/5437056/diff/3005/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:386: if blanks == 2 && len(lines) > i+2 &&
isBlank(lines[i+1]) && !isBlank(lines[i+2]) {
s/>/>=/

http://codereview.appspot.com/5437056/diff/3005/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:392: template.HTMLEscape(w, line)
Just to make the output a bit nicer, please put the \n into html_endh and make
this:
  template.HTMLEscape(w, line[:len(line)-1])

http://codereview.appspot.com/5437056/diff/3005/src/pkg/go/doc/comment_test.go
File src/pkg/go/doc/comment_test.go (right):

http://codereview.appspot.com/5437056/diff/3005/src/pkg/go/doc/comment_test.g...
src/pkg/go/doc/comment_test.go:11: func TestIsHeading(t *testing.T) {
It'd be cool to add a test for ToHTML also.

[adg, 2011-11-28 05:34:41]

http://codereview.appspot.com/5437056/diff/3005/src/pkg/go/doc/comment.go
File src/pkg/go/doc/comment.go (right):

http://codereview.appspot.com/5437056/diff/3005/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:287: // len(s) >= 2
On 2011/11/28 05:33:44, adg wrote:
> drop this comment

Just saw that gri asked you to put it there. It's his code, please leave it.

[volker.dobler, 2011-11-28 09:01:16]

Hello golang-dev@googlegroups.com, jan.mercl@nic.cz, gri@golang.org,
adg@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[volker.dobler, 2011-11-28 09:02:24]

http://codereview.appspot.com/5437056/diff/3005/src/pkg/go/doc/comment.go
File src/pkg/go/doc/comment.go (right):

http://codereview.appspot.com/5437056/diff/3005/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:275: const maxWordsInHeading = 5 // a heading has at
most that many words
On 2011/11/28 05:33:44, adg wrote:
> I don't think we need this heuristic. I can't think of a correctly-formatted
> single line that the other heuristics would not catch.

I added this heuristic to prevent what some people where
afraid of:  Suddenly lots of unwanted, stray headings
just because a newline slipt into the comment or someone
was not aware of the "invisible" markup used for detecting
headings.

I personally would go even further: A maximum of 3 words,
or the full line is capitalized (with the usual exemptions
like a, it, and, ...) just to be perfectly safe, but
requiering capitalization looks like markup which is
of limits.

Even if it is not needed: It doesn't hurt to check and
it adds IMHO additional safety in case of a uncorrectly
formatted single line to the heuristics.

Really remove? Or mabye just relax to 6 or even 7 words?

http://codereview.appspot.com/5437056/diff/3005/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:283: func isHeading(s []byte) bool {
On 2011/11/28 05:33:44, adg wrote:
> s/s/line/
> to make it clear what the function expects

Done.

http://codereview.appspot.com/5437056/diff/3005/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:287: // len(s) >= 2
On 2011/11/28 05:34:41, adg wrote:
> On 2011/11/28 05:33:44, adg wrote:
> > drop this comment
> 
> Just saw that gri asked you to put it there. It's his code, please leave it.

IMHO it is overkill: The if is just three lines away
and in the absence of operator overloading it's clear
in any case that not(n < 2)  <==>  n >= 2. (Even with
operator overloading as len() returns an int).

http://codereview.appspot.com/5437056/diff/3005/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:322: // Enclose headings into <h3> tags.
On 2011/11/28 05:33:44, adg wrote:
> s/into/with/

Done.

http://codereview.appspot.com/5437056/diff/3005/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:386: if blanks == 2 && len(lines) > i+2 &&
isBlank(lines[i+1]) && !isBlank(lines[i+2]) {
On 2011/11/28 05:33:44, adg wrote:
> s/>/>=/

Done.

http://codereview.appspot.com/5437056/diff/3005/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:392: template.HTMLEscape(w, line)
On 2011/11/28 05:33:44, adg wrote:
> Just to make the output a bit nicer, please put the \n into html_endh and make
> this:
>   template.HTMLEscape(w, line[:len(line)-1])

Done.

[adg, 2011-11-28 09:44:41]

On 28 November 2011 20:02,  <dr.volker.dobler@gmail.com> wrote:
>
> http://codereview.appspot.com/5437056/diff/3005/src/pkg/go/doc/comment.go
> File src/pkg/go/doc/comment.go (right):
>
>
http://codereview.appspot.com/5437056/diff/3005/src/pkg/go/doc/comment.go#new...
> src/pkg/go/doc/comment.go:275: const maxWordsInHeading = 5 // a heading
> has at most that many words
> On 2011/11/28 05:33:44, adg wrote:
>>
>> I don't think we need this heuristic. I can't think of a
>
> correctly-formatted
>>
>> single line that the other heuristics would not catch.
>
> I added this heuristic to prevent what some people where
> afraid of:  Suddenly lots of unwanted, stray headings
> just because a newline slipt into the comment or someone
> was not aware of the "invisible" markup used for detecting
> headings.

I don't think it's a significant risk.

To test it, I wrote a tool to scan a go source tree for headings that
match the heuristic. Build the attached program and run it in the
directory you want to scan.

Here's it's output on the Go tree:

2011/11/28 20:39:40 src/cmd/goinstall: <h3>Remote Repositories</h3>
2011/11/28 20:39:40 src/cmd/goinstall: <h3>The GOPATH Environment Variable</h3>
2011/11/28 20:39:40 src/cmd/prof: <h3>Usage: prof -p pid [-t
total_secs] [-d delta_msec] [6.out args ...]</h3>
2011/11/28 20:39:41 src/pkg/exp/gotype: <h3>Examples</h3>
2011/11/28 20:39:42 src/pkg/html/template: <h3>Introduction</h3>
2011/11/28 20:39:42 src/pkg/html/template: <h3>Contexts</h3>
2011/11/28 20:39:42 src/pkg/html/template: <h3>Errors</h3>
2011/11/28 20:39:42 src/pkg/html/template: <h3>A fuller picture</h3>
2011/11/28 20:39:42 src/pkg/html/template: <h3>Contexts</h3>
2011/11/28 20:39:42 src/pkg/html/template: <h3>Typed Strings</h3>
2011/11/28 20:39:42 src/pkg/html/template: <h3>Security Model</h3>

The only mismatch is the usage line in cmd/pprof. Note that if the
usage line was shorter it would fall inside a 5-word limit, and that
"The GOPATH Environment Variable" does not fall inside your preferred
3-word limit.

Here's what I got for my $GOPATH tree:

2011/11/28 20:41:48 src/appengine/datastore: <h3>Basic Operations</h3>
2011/11/28 20:41:48 src/appengine/datastore: <h3>Properties</h3>
2011/11/28 20:41:48 src/appengine/datastore: <h3>Queries</h3>
2011/11/28 20:41:48 src/appengine/datastore: <h3>Transactions</h3>

Only correct matches over dozens of packages.

To fix the pprof usage situation, we should discard all lines that end
in non-alpha/non-whitespace characters. I think that would suffice.

Andrew

[volker.dobler, 2011-11-28 09:57:50]

Hello jan.mercl@nic.cz, gri@golang.org, adg@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[volker.dobler, 2011-11-28 10:01:06]

On 2011/11/28 09:44:41, adg wrote:
> On 28 November 2011 20:02,  <mailto:dr.volker.dobler@gmail.com> wrote:
> >
> > http://codereview.appspot.com/5437056/diff/3005/src/pkg/go/doc/comment.go
> > File src/pkg/go/doc/comment.go (right):
> >
> >
>
http://codereview.appspot.com/5437056/diff/3005/src/pkg/go/doc/comment.go#new...
> > src/pkg/go/doc/comment.go:275: const maxWordsInHeading = 5 // a heading
> > has at most that many words
> > On 2011/11/28 05:33:44, adg wrote:
> >>
> >> I don't think we need this heuristic. I can't think of a
> >
> > correctly-formatted
> >>
> >> single line that the other heuristics would not catch.
> >
> > I added this heuristic to prevent what some people where
> > afraid of: &nbsp;Suddenly lots of unwanted, stray headings
> > just because a newline slipt into the comment or someone
> > was not aware of the "invisible" markup used for detecting
> > headings.
> 
> I don't think it's a significant risk.
> 
> To test it, I wrote a tool to scan a go source tree for headings that
> match the heuristic. Build the attached program and run it in the
> directory you want to scan.
> 
> Here's it's output on the Go tree:
> 
> 2011/11/28 20:39:40 src/cmd/goinstall: <h3>Remote Repositories</h3>
> 2011/11/28 20:39:40 src/cmd/goinstall: <h3>The GOPATH Environment
Variable</h3>
> 2011/11/28 20:39:40 src/cmd/prof: <h3>Usage: prof -p pid [-t
> total_secs] [-d delta_msec] [6.out args ...]</h3>
> 2011/11/28 20:39:41 src/pkg/exp/gotype: <h3>Examples</h3>
> 2011/11/28 20:39:42 src/pkg/html/template: <h3>Introduction</h3>
> 2011/11/28 20:39:42 src/pkg/html/template: <h3>Contexts</h3>
> 2011/11/28 20:39:42 src/pkg/html/template: <h3>Errors</h3>
> 2011/11/28 20:39:42 src/pkg/html/template: <h3>A fuller picture</h3>
> 2011/11/28 20:39:42 src/pkg/html/template: <h3>Contexts</h3>
> 2011/11/28 20:39:42 src/pkg/html/template: <h3>Typed Strings</h3>
> 2011/11/28 20:39:42 src/pkg/html/template: <h3>Security Model</h3>
> 
> The only mismatch is the usage line in cmd/pprof. Note that if the
> usage line was shorter it would fall inside a 5-word limit, and that
> "The GOPATH Environment Variable" does not fall inside your preferred
> 3-word limit.
> 
> Here's what I got for my $GOPATH tree:
> 
> 2011/11/28 20:41:48 src/appengine/datastore: <h3>Basic Operations</h3>
> 2011/11/28 20:41:48 src/appengine/datastore: <h3>Properties</h3>
> 2011/11/28 20:41:48 src/appengine/datastore: <h3>Queries</h3>
> 2011/11/28 20:41:48 src/appengine/datastore: <h3>Transactions</h3>
> 
> Only correct matches over dozens of packages.
> 
> To fix the pprof usage situation, we should discard all&nbsp;lines that end
> in non-alpha/non-whitespace characters. I think that would suffice.
> 
> Andrew

Measurement always wins :-)
Removed and replaced with a "last is letter" check.
Volker

[adg, 2011-11-28 10:01:17]

http://codereview.appspot.com/5437056/diff/8001/src/pkg/go/doc/comment.go
File src/pkg/go/doc/comment.go (right):

http://codereview.appspot.com/5437056/diff/8001/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:173: html_h      = []byte("<h3>") // "Overview",
"Constants", function and types are h2
I think you can leave this comment out.

http://codereview.appspot.com/5437056/diff/8001/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:278: // The heuristics assumes that a heading:
don't forget to amend this comment

http://codereview.appspot.com/5437056/diff/8001/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:284: if len(line) < 2 {
The body of this function could now be:

        s := utf8.NewString(string(bytes.TrimSpace(line)))
        if s.RuneCount() == 0 {
                return false
        }

        // starts with uppercase letter
        if r := s.At(0); !unicode.IsLetter(r) || !unicode.IsUpper(r) {
                return false
        }

        // ends with a letter or digit
        if r := s.At(s.RuneCount()-1); !unicode.IsLetter(r) &&
!unicode.IsDigit(r) {
                return false
        }

        return true

[adg, 2011-11-28 10:02:07]

http://codereview.appspot.com/5437056/diff/11001/src/pkg/go/doc/comment.go
File src/pkg/go/doc/comment.go (right):

http://codereview.appspot.com/5437056/diff/11001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:375: // current line is nonempyt, preceeded by two
blank lines and
s/nonempyt/nonempty/

please wrap the comment at 80 columns

[adg, 2011-11-28 10:04:10]

http://codereview.appspot.com/5437056/diff/11001/src/pkg/go/doc/comment.go
File src/pkg/go/doc/comment.go (right):

http://codereview.appspot.com/5437056/diff/11001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:285: line = line[:len(line)-1]
What you've done here is probably faster than what I suggested (using
unicode.String).

But at the top of the function, do:
  line = bytes.TrimSpace(line)
  if len(line) == 0 { return false }

http://codereview.appspot.com/5437056/diff/11001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:295: if !unicode.IsLetter(r) {
also test for a digit

[volker.dobler, 2011-11-28 11:07:22]

Hello jan.mercl@nic.cz, gri@golang.org, adg@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[volker.dobler, 2011-11-28 11:07:42]

http://codereview.appspot.com/5437056/diff/11001/src/pkg/go/doc/comment.go
File src/pkg/go/doc/comment.go (right):

http://codereview.appspot.com/5437056/diff/11001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:285: line = line[:len(line)-1]
On 2011/11/28 10:04:10, adg wrote:
> What you've done here is probably faster than what I suggested (using
> unicode.String).
> 
> But at the top of the function, do:
>   line = bytes.TrimSpace(line)
>   if len(line) == 0 { return false }

Done.

http://codereview.appspot.com/5437056/diff/11001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:295: if !unicode.IsLetter(r) {
On 2011/11/28 10:04:10, adg wrote:
> also test for a digit

Done.

http://codereview.appspot.com/5437056/diff/11001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:375: // current line is nonempyt, preceeded by two
blank lines and
On 2011/11/28 10:02:07, adg wrote:
> s/nonempyt/nonempty/
> 
> please wrap the comment at 80 columns

Done.

[rsc, 2011-11-28 17:15:53]

Do you have some examples of before/after HTML that we can view?

Thanks.

http://codereview.appspot.com/5437056/diff/11004/src/pkg/go/doc/comment.go
File src/pkg/go/doc/comment.go (right):

http://codereview.appspot.com/5437056/diff/11004/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:70: // interior blank lines to at most two blank
lines.
Why is it necessary to change this?
I'd prefer not to.

http://codereview.appspot.com/5437056/diff/11004/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:293: if !(unicode.IsLetter(r) || unicode.IsDigit(r)) {
push the ! through.
if !unicode.IsLetter(r) && !unicode.IsDigit(r)

http://codereview.appspot.com/5437056/diff/11004/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:373: // current line is nonempty, preceeded by two
blank
s/ee/e/

One blank line seems fine.

http://codereview.appspot.com/5437056/diff/11004/src/pkg/go/doc/comment_test.go
File src/pkg/go/doc/comment_test.go (right):

http://codereview.appspot.com/5437056/diff/11004/src/pkg/go/doc/comment_test....
src/pkg/go/doc/comment_test.go:12: for _, heading := range []string{"Section",
"A typical usage", "ΔΛΞ is Greek", "Foo 42"} {
please make this look like the other table-driven tests in the tree.

at top level
var isHeadingTests = []struct {
    in string
    out bool
}{
    {"Section", true},
    {"A typical usage", true},
...
}

then here

for _, tt := range isHeadingTests {
    if out := isHeading([]byte(tt.in)); out != tt.out {
        t.Errorf("isHeading(%q) = %v, want %v", tt.in, out, tt.out)
    }
}

[volker.dobler, 2011-11-28 18:59:26]

Hello jan.mercl@nic.cz, gri@golang.org, adg@golang.org, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[volker.dobler, 2011-11-28 19:19:43]

On 11/28/11, rsc@golang.org <rsc@golang.org> wrote:
> Do you have some examples of before/after HTML that we can view?

Please see the attached screenshot.

[volker.dobler, 2011-11-28 19:20:32]

http://codereview.appspot.com/5437056/diff/11004/src/pkg/go/doc/comment.go
File src/pkg/go/doc/comment.go (right):

http://codereview.appspot.com/5437056/diff/11004/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:70: // interior blank lines to at most two blank
lines.
On 2011/11/28 17:15:53, rsc wrote:
> Why is it necessary to change this?
> I'd prefer not to.

If this additional blank line is not preserved
here it cannot be used below to determine headings.
Determination of heading based on the content only of 
a single line (as implemented in isHeading()) would
falsely identify in (html/template/doc.go#L143)
// 
// The template
//
//   Hello, {{.}}!
//
// can be invoked with
"The template" as a heading which it isn't.

If only one blank line is kept than the heuristic
for headings must be strengthened e.g. by requiring
captitalization ("The Template") which (a) has the
semll of markup and (b) is hard to do right as articles
and short prepositions (in, at, on, ...) and conjuctions
(and, as, ...) should not be not capitalized in headings.

Keeping two lines here helps detecting headings reliable.
These two blank lines will appear in the output of godocs
text version which is nicer for headings anyway.

http://codereview.appspot.com/5437056/diff/11004/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:293: if !(unicode.IsLetter(r) || unicode.IsDigit(r)) {
On 2011/11/28 17:15:53, rsc wrote:
> push the ! through.
> if !unicode.IsLetter(r) && !unicode.IsDigit(r)

Done.

http://codereview.appspot.com/5437056/diff/11004/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:373: // current line is nonempty, preceeded by two
blank
On 2011/11/28 17:15:53, rsc wrote:
> s/ee/e/
Done
 
> One blank line seems fine.
See above.

http://codereview.appspot.com/5437056/diff/11004/src/pkg/go/doc/comment_test.go
File src/pkg/go/doc/comment_test.go (right):

http://codereview.appspot.com/5437056/diff/11004/src/pkg/go/doc/comment_test....
src/pkg/go/doc/comment_test.go:12: for _, heading := range []string{"Section",
"A typical usage", "ΔΛΞ is Greek", "Foo 42"} {
On 2011/11/28 17:15:53, rsc wrote:
> please make this look like the other table-driven tests in the tree.
> 
> at top level
> var isHeadingTests = []struct {
>     in string
>     out bool
> }{
>     {"Section", true},
>     {"A typical usage", true},
> ...
> }
> 
> then here
> 
> for _, tt := range isHeadingTests {
>     if out := isHeading([]byte(tt.in)); out != tt.out {
>         t.Errorf("isHeading(%q) = %v, want %v", tt.in, out, tt.out)
>     }
> }
> 

Done.

[rsc, 2011-11-28 19:23:30]

On Mon, Nov 28, 2011 at 14:20,  <dr.volker.dobler@gmail.com> wrote:
> Keeping two lines here helps detecting headings reliable.
> These two blank lines will appear in the output of godocs
> text version which is nicer for headings anyway.

Okay.  Can you run this over the package docs for the tree
and make a list of the headings it identifiers?

Thanks.
Russ

[volker.dobler, 2011-11-28 19:36:18]

Just running Andrew's script (tmp/headscan) produces:

volker@linuxvm:~/go/src $ ../../tmp/headscan
2011/11/28 20:32:35 cmd/goinstall: <h3>Remote Repositories</h3>
2011/11/28 20:32:35 cmd/goinstall: <h3>The GOPATH Environment Variable</h3>
2011/11/28 20:32:36 pkg/exp/gotype: <h3>Examples</h3>
2011/11/28 20:32:37 pkg/html/template: <h3>Introduction</h3>
2011/11/28 20:32:37 pkg/html/template: <h3>Contexts</h3>
2011/11/28 20:32:37 pkg/html/template: <h3>Errors</h3>
2011/11/28 20:32:37 pkg/html/template: <h3>A fuller picture</h3>
2011/11/28 20:32:37 pkg/html/template: <h3>Contexts</h3>
2011/11/28 20:32:37 pkg/html/template: <h3>Typed Strings</h3>
2011/11/28 20:32:37 pkg/html/template: <h3>Security Model</h3>

Which is (mod prof) the same list as Andrew generated: Not that much
and no obviuos false positives.


On 11/28/11, Russ Cox <rsc@golang.org> wrote:
> On Mon, Nov 28, 2011 at 14:20,  <dr.volker.dobler@gmail.com> wrote:
>> Keeping two lines here helps detecting headings reliable.
>> These two blank lines will appear in the output of godocs
>> text version which is nicer for headings anyway.
>
> Okay.  Can you run this over the package docs for the tree
> and make a list of the headings it identifiers?
>
> Thanks.
> Russ
>


-- 
Dr. Volker Dobler

[rsc, 2011-11-28 19:38:01]

Thanks, seems reasonable to me.
Leaving for Andrew.

[adg, 2011-11-28 22:47:04]

Looking good. One last thing.

http://codereview.appspot.com/5437056/diff/13002/src/pkg/go/doc/comment.go
File src/pkg/go/doc/comment.go (right):

http://codereview.appspot.com/5437056/diff/13002/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:374: // lines and followed by a non-blank line: this
might
This comment is incorrect.

Current line is non-blank, preceded by two blanks, and followed by a blank and a
non-blank.

[adg, 2011-11-28 22:47:47]

Also please update the CL description to reflect the decisions made. (hg change
5437056)

[gri, 2011-11-28 22:52:48]

Hold on, please. I am not yet convinced. The heuristic as is leads people to
format for it, instead of the heuristic doing the right thing automatically.

Please apply the following changes.

http://codereview.appspot.com/5437056/diff/13002/src/pkg/go/doc/comment.go
File src/pkg/go/doc/comment.go (right):

http://codereview.appspot.com/5437056/diff/13002/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:174: html_h      = []byte("<h3>")
I think h3 is too large. I tried to this on my system and the headings (e.g.
text/template) scream at me. h4 seems good. It also matches the size of the
"Package files" section.

http://codereview.appspot.com/5437056/diff/13002/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:372: if blanks == 2 && len(lines) >= i+2 &&
isBlank(lines[i+1]) && !isBlank(lines[i+2]) {
I think this (2) is too high. I cannot make this work with many of the existing
comments w/o reformatting them and inserting two empty lines before a heading.
While I personally don't mind that style, it's not the prevalent style
especially now that we have at most one empty line in code. I think it should be
one.

[gri, 2011-11-28 22:57:08]

http://codereview.appspot.com/5437056/diff/13002/src/pkg/go/doc/comment.go
File src/pkg/go/doc/comment.go (right):

http://codereview.appspot.com/5437056/diff/13002/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:308: // Enclose headings with <h3> tags.
s/<h3>/header/

so the comment is not dependent of the value of the constants

http://codereview.appspot.com/5437056/diff/13002/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:373: // current line is nonempty, preceded by two
blank
one blank line

[adg, 2011-11-28 23:01:54]

http://codereview.appspot.com/5437056/diff/13002/src/pkg/go/doc/comment.go
File src/pkg/go/doc/comment.go (right):

http://codereview.appspot.com/5437056/diff/13002/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:372: if blanks == 2 && len(lines) >= i+2 &&
isBlank(lines[i+1]) && !isBlank(lines[i+2]) {
On 2011/11/28 22:52:48, gri wrote:
> I think this (2) is too high. I cannot make this work with many of the
existing
> comments w/o reformatting them and inserting two empty lines before a heading.
> While I personally don't mind that style, it's not the prevalent style
> especially now that we have at most one empty line in code. I think it should
be
> one.

I think this heuristic is a reasonable expectation. We already require people to
indent to achieve fixed formatting. I think it's okay to expect two blank lines
before a heading. It looks nicer as a plain comment, too.

These are the matches with only one blank line. As you can see, there are many
cases where this is problematic.

2011/11/29 09:59:59 cmd/cgo: <h3>Usage: cgo [compiler options] file.go</h3>
2011/11/29 09:59:59 cmd/gofmt: <h3>Examples</h3>
2011/11/29 09:59:59 cmd/goinstall: <h3>Another common idiom is to use</h3>
2011/11/29 09:59:59 cmd/goinstall: <h3>Remote Repositories</h3>
2011/11/29 09:59:59 cmd/goinstall: <h3>For code hosted on other servers,
goinstall recognizes the general form</h3>
2011/11/29 09:59:59 cmd/goinstall: <h3>The GOPATH Environment Variable</h3>
2011/11/29 09:59:59 pkg/archive/zip: <h3>See:
http://www.pkware.com/documents/casestudies/APPNOTE.TXT</h3>
2011/11/29 10:00:00 pkg/encoding/csv: <h3>Newlines and commas may be included in
a quoted-field</h3>
2011/11/29 10:00:00 pkg/encoding/gob: <h3>In summary, a gob stream looks
like</h3>
2011/11/29 10:00:00 pkg/exp/gotype: <h3>Examples</h3>
2011/11/29 10:00:01 pkg/html/template: <h3>Introduction</h3>
2011/11/29 10:00:01 pkg/html/template: <h3>Example</h3>
2011/11/29 10:00:01 pkg/html/template: <h3>Contexts</h3>
2011/11/29 10:00:01 pkg/html/template: <h3>Errors</h3>
2011/11/29 10:00:01 pkg/html/template: <h3>A fuller picture</h3>
2011/11/29 10:00:01 pkg/html/template: <h3>Contexts</h3>
2011/11/29 10:00:01 pkg/html/template: <h3>Typed Strings</h3>
2011/11/29 10:00:01 pkg/html/template: <h3>The template</h3>
2011/11/29 10:00:01 pkg/html/template: <h3>Security Model</h3>
2011/11/29 10:00:03 pkg/text/template: <h3>Actions</h3>
2011/11/29 10:00:03 pkg/text/template: <h3>Arguments</h3>
2011/11/29 10:00:03 pkg/text/template: <h3>Pipelines</h3>
2011/11/29 10:00:03 pkg/text/template: <h3>Variables</h3>
2011/11/29 10:00:03 pkg/text/template: <h3>Examples</h3>
2011/11/29 10:00:03 pkg/text/template: <h3>Functions</h3>
2011/11/29 10:00:03 pkg/text/template: <h3>Associated templates</h3>
2011/11/29 10:00:03 pkg/text/template: <h3>Nested template definitions</h3>

[gri, 2011-11-29 00:17:19]

Here's a summary of a longer discussion between Andrew and me:

We both agree that the comments look nicer and are more readable with
slightly (h4 I think) highlighted section headers. This is mainly
important for the long documentation of tools.

Unfortunately, as it is, the highlighting won't work on most (all?)
existing code because headers don't have two empty lines before them.

I strongly disagree with the requirement of 2 empty lines before a
section header - it will inevitably lead to many CLs "fixing" existing
comments to make use of the new "feature". This is a very slippery
slope. There are many such "features" that could be introduced for
"nicer" comments (italics, colors, etc.) and they all might "improve"
comments. But they all add extra rules, and they all detract from the
main concern which is to write concise, clear, and correct comments.
Experience shows that those concerns will take a backseat in favor of
"nice" formatting.

Andrew correctly points out that w/o the 2-line rule, we get several
false positives. I believe that if we are to introduce a heuristic for
nicer comment formatting, it has to "just work" w/o the author having
to worry about formatting rules. gofmt (and thus godoc) have
significant amounts of heuristic for good comment formatting already,
and we have spent much time to make them just work and the mechanisms
behind it are effectively invisible.

I propose the following change to the current heuristic:

1) one empty line before and after a header - this is a minimum
requirement that is already followed and authors tend to do naturally
2) no special chars in a header (exactly what this means is tbd, but
we can fine-tune) - this will exclude several cases that now are false
positives. after all a header is a title - it should be clean and
devoid of excess punctuation
3) no header if it is immediately followed by an indented section
(formatted as <pre>) - this will exclude the remaining false positives

1) and 2) are trivial changes to the current CL. 3) may require a bit more work.

I want the mechanism to work really well on existing code w/o any
changes and thus be "invisible" to the author of comments. Even if
that means that the heuristic is a bit more complex.

If we cannot make this work well, I am against this CL.
- gri






On Mon, Nov 28, 2011 at 3:01 PM,  <adg@golang.org> wrote:
>
> http://codereview.appspot.com/5437056/diff/13002/src/pkg/go/doc/comment.go
> File src/pkg/go/doc/comment.go (right):
>
>
http://codereview.appspot.com/5437056/diff/13002/src/pkg/go/doc/comment.go#ne...
> src/pkg/go/doc/comment.go:372: if blanks == 2 && len(lines) >= i+2 &&
> isBlank(lines[i+1]) && !isBlank(lines[i+2]) {
> On 2011/11/28 22:52:48, gri wrote:
>>
>> I think this (2) is too high. I cannot make this work with many of the
>
> existing
>>
>> comments w/o reformatting them and inserting two empty lines before a
>
> heading.
>>
>> While I personally don't mind that style, it's not the prevalent style
>> especially now that we have at most one empty line in code. I think it
>
> should be
>>
>> one.
>
> I think this heuristic is a reasonable expectation. We already require
> people to indent to achieve fixed formatting. I think it's okay to
> expect two blank lines before a heading. It looks nicer as a plain
> comment, too.
>
> These are the matches with only one blank line. As you can see, there
> are many cases where this is problematic.
>
> 2011/11/29 09:59:59 cmd/cgo: <h3>Usage: cgo [compiler options]
> file.go</h3>
> 2011/11/29 09:59:59 cmd/gofmt: <h3>Examples</h3>
> 2011/11/29 09:59:59 cmd/goinstall: <h3>Another common idiom is to
> use</h3>
> 2011/11/29 09:59:59 cmd/goinstall: <h3>Remote Repositories</h3>
> 2011/11/29 09:59:59 cmd/goinstall: <h3>For code hosted on other servers,
> goinstall recognizes the general form</h3>
> 2011/11/29 09:59:59 cmd/goinstall: <h3>The GOPATH Environment
> Variable</h3>
> 2011/11/29 09:59:59 pkg/archive/zip: <h3>See:
> http://www.pkware.com/documents/casestudies/APPNOTE.TXT</h3>
> 2011/11/29 10:00:00 pkg/encoding/csv: <h3>Newlines and commas may be
> included in a quoted-field</h3>
> 2011/11/29 10:00:00 pkg/encoding/gob: <h3>In summary, a gob stream looks
> like</h3>
> 2011/11/29 10:00:00 pkg/exp/gotype: <h3>Examples</h3>
> 2011/11/29 10:00:01 pkg/html/template: <h3>Introduction</h3>
> 2011/11/29 10:00:01 pkg/html/template: <h3>Example</h3>
> 2011/11/29 10:00:01 pkg/html/template: <h3>Contexts</h3>
> 2011/11/29 10:00:01 pkg/html/template: <h3>Errors</h3>
> 2011/11/29 10:00:01 pkg/html/template: <h3>A fuller picture</h3>
> 2011/11/29 10:00:01 pkg/html/template: <h3>Contexts</h3>
> 2011/11/29 10:00:01 pkg/html/template: <h3>Typed Strings</h3>
> 2011/11/29 10:00:01 pkg/html/template: <h3>The template</h3>
> 2011/11/29 10:00:01 pkg/html/template: <h3>Security Model</h3>
> 2011/11/29 10:00:03 pkg/text/template: <h3>Actions</h3>
> 2011/11/29 10:00:03 pkg/text/template: <h3>Arguments</h3>
> 2011/11/29 10:00:03 pkg/text/template: <h3>Pipelines</h3>
> 2011/11/29 10:00:03 pkg/text/template: <h3>Variables</h3>
> 2011/11/29 10:00:03 pkg/text/template: <h3>Examples</h3>
> 2011/11/29 10:00:03 pkg/text/template: <h3>Functions</h3>
> 2011/11/29 10:00:03 pkg/text/template: <h3>Associated templates</h3>
> 2011/11/29 10:00:03 pkg/text/template: <h3>Nested template
> definitions</h3>
>
> http://codereview.appspot.com/5437056/
>

[volker.dobler, 2011-11-29 05:54:33]

On Tue, Nov 29, 2011 at 1:17 AM, Robert Griesemer <gri@golang.org> wrote:

> Here's a summary of a longer discussion between Andrew and me:
>
> We both agree that the comments look nicer and are more readable with
> slightly (h4 I think) highlighted section headers. This is mainly
> important for the long documentation of tools.
>
> Unfortunately, as it is, the highlighting won't work on most (all?)
> existing code because headers don't have two empty lines before them.
>
> I strongly disagree with the requirement of 2 empty lines before a
> section header - it will inevitably lead to many CLs "fixing" existing
> comments to make use of the new "feature". This is a very slippery
> slope. There are many such "features" that could be introduced for
> "nicer" comments (italics, colors, etc.) and they all might "improve"
> comments. But they all add extra rules, and they all detract from the
> main concern which is to write concise, clear, and correct comments.
> Experience shows that those concerns will take a backseat in favor of
> "nice" formatting.
>
> Andrew correctly points out that w/o the 2-line rule, we get several
> false positives. I believe that if we are to introduce a heuristic for
> nicer comment formatting, it has to "just work" w/o the author having
> to worry about formatting rules. gofmt (and thus godoc) have
> significant amounts of heuristic for good comment formatting already,
> and we have spent much time to make them just work and the mechanisms
> behind it are effectively invisible.
>
> I propose the following change to the current heuristic:
>
> 1) one empty line before and after a header - this is a minimum
> requirement that is already followed and authors tend to do naturally
> 2) no special chars in a header (exactly what this means is tbd, but
> we can fine-tune) - this will exclude several cases that now are false
> positives. after all a header is a title - it should be clean and
> devoid of excess punctuation
> 3) no header if it is immediately followed by an indented section
> (formatted as <pre>) - this will exclude the remaining false positives
>
> 1) and 2) are trivial changes to the current CL. 3) may require a bit more
> work.
>
> I want the mechanism to work really well on existing code w/o any
> changes and thus be "invisible" to the author of comments. Even if
> that means that the heuristic is a bit more complex.
>
> If we cannot make this work well, I am against this CL.
> - gri
>

That sounds very reasonable and I'll implement and try it out.
This may take some time.

Regarding the h4 vs h3? Shouldn't just the CSS for h3 changed to
smaller size? Normally you try to keep the hierarchy and
not skip headings like
<h2>Overview</h2>
Text
<h4>Some Heading</h4>

Volker


>
>
>
>
> On Mon, Nov 28, 2011 at 3:01 PM,  <adg@golang.org> wrote:
> >
> >
> http://codereview.appspot.com/5437056/diff/13002/src/pkg/go/doc/comment.go
> > File src/pkg/go/doc/comment.go (right):
> >
> >
>
http://codereview.appspot.com/5437056/diff/13002/src/pkg/go/doc/comment.go#ne...
> > src/pkg/go/doc/comment.go:372: if blanks == 2 && len(lines) >= i+2 &&
> > isBlank(lines[i+1]) && !isBlank(lines[i+2]) {
> > On 2011/11/28 22:52:48, gri wrote:
> >>
> >> I think this (2) is too high. I cannot make this work with many of the
> >
> > existing
> >>
> >> comments w/o reformatting them and inserting two empty lines before a
> >
> > heading.
> >>
> >> While I personally don't mind that style, it's not the prevalent style
> >> especially now that we have at most one empty line in code. I think it
> >
> > should be
> >>
> >> one.
> >
> > I think this heuristic is a reasonable expectation. We already require
> > people to indent to achieve fixed formatting. I think it's okay to
> > expect two blank lines before a heading. It looks nicer as a plain
> > comment, too.
> >
> > These are the matches with only one blank line. As you can see, there
> > are many cases where this is problematic.
> >
> > 2011/11/29 09:59:59 cmd/cgo: <h3>Usage: cgo [compiler options]
> > file.go</h3>
> > 2011/11/29 09:59:59 cmd/gofmt: <h3>Examples</h3>
> > 2011/11/29 09:59:59 cmd/goinstall: <h3>Another common idiom is to
> > use</h3>
> > 2011/11/29 09:59:59 cmd/goinstall: <h3>Remote Repositories</h3>
> > 2011/11/29 09:59:59 cmd/goinstall: <h3>For code hosted on other servers,
> > goinstall recognizes the general form</h3>
> > 2011/11/29 09:59:59 cmd/goinstall: <h3>The GOPATH Environment
> > Variable</h3>
> > 2011/11/29 09:59:59 pkg/archive/zip: <h3>See:
> > http://www.pkware.com/documents/casestudies/APPNOTE.TXT</h3>
> > 2011/11/29 10:00:00 pkg/encoding/csv: <h3>Newlines and commas may be
> > included in a quoted-field</h3>
> > 2011/11/29 10:00:00 pkg/encoding/gob: <h3>In summary, a gob stream looks
> > like</h3>
> > 2011/11/29 10:00:00 pkg/exp/gotype: <h3>Examples</h3>
> > 2011/11/29 10:00:01 pkg/html/template: <h3>Introduction</h3>
> > 2011/11/29 10:00:01 pkg/html/template: <h3>Example</h3>
> > 2011/11/29 10:00:01 pkg/html/template: <h3>Contexts</h3>
> > 2011/11/29 10:00:01 pkg/html/template: <h3>Errors</h3>
> > 2011/11/29 10:00:01 pkg/html/template: <h3>A fuller picture</h3>
> > 2011/11/29 10:00:01 pkg/html/template: <h3>Contexts</h3>
> > 2011/11/29 10:00:01 pkg/html/template: <h3>Typed Strings</h3>
> > 2011/11/29 10:00:01 pkg/html/template: <h3>The template</h3>
> > 2011/11/29 10:00:01 pkg/html/template: <h3>Security Model</h3>
> > 2011/11/29 10:00:03 pkg/text/template: <h3>Actions</h3>
> > 2011/11/29 10:00:03 pkg/text/template: <h3>Arguments</h3>
> > 2011/11/29 10:00:03 pkg/text/template: <h3>Pipelines</h3>
> > 2011/11/29 10:00:03 pkg/text/template: <h3>Variables</h3>
> > 2011/11/29 10:00:03 pkg/text/template: <h3>Examples</h3>
> > 2011/11/29 10:00:03 pkg/text/template: <h3>Functions</h3>
> > 2011/11/29 10:00:03 pkg/text/template: <h3>Associated templates</h3>
> > 2011/11/29 10:00:03 pkg/text/template: <h3>Nested template
> > definitions</h3>
> >
> > http://codereview.appspot.com/5437056/
> >
>



-- 
Dr. Volker Dobler

[gri, 2011-11-29 06:00:04]

On Mon, Nov 28, 2011 at 9:54 PM, Volker Dobler
<dr.volker.dobler@gmail.com> wrote:
> Regarding the h4 vs h3? Shouldn't just the CSS for h3 changed to
> smaller size? Normally you try to keep the hierarchy and
> not skip headings like
> <h2>Overview</h2>
> Text
> <h4>Some Heading</h4>

Agreed. I am just observing that at the moment h4 looks better then
h3. Maybe it's the CSS that's wrong.

There's no rush on this CL. It would be good to include Andrew's
script in some form so it's easy to experiment down the road if
changes are necessary.

- gri

[volker.dobler, 2011-11-29 22:14:03]

Hello jan.mercl@nic.cz, gri@golang.org, adg@golang.org, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[gri, 2011-11-30 20:13:54]

Much better. This heuristic can be relatively easily refined as necessary.

Andrew's headscan file is missing. Regarding testing: We don't want to have to
update a file with rejected or accepted headings when new sources are added ot
existing sources are modified. Incorrect formatting is not breaking the build,
and we will just fix it when we see it. The headscan tool is useful to test
tweaks to the algorithm though, and we want to have it available.

Some nitpicks and some suggestions for simplifications.

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/Makefile
File src/pkg/go/doc/Makefile (right):

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/Makefile#newc...
src/pkg/go/doc/Makefile:19: headscan.$O: headscan.go
headscan.go is missing from this CL

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go
File src/pkg/go/doc/comment.go (right):

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:276: // Check if line passes the heuristics for a
valid heading.
just:

// heading returns the (possibly trimmed) line if it passes as a valid section
heading;
// otherwise it returns nil.

Leave the rest of the comment away: 1) there is detailed comments on these
requirements in the code, 2) this is not an exported functions, and 3) the
heuristic may change over time and this comment is likely to get out of sync.

I would also leave the TODO away. Over time we will see if adjustments are
needed and we will just make them.

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:296: // starts with uppercase letter
// a heading must start with an uppercase letter

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:302: // ends in a letter or digit
// it must end in a letter, digit, or ':'

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:308: // strip trailing : (which is allowed)
more direct and faster, since we have r already:

// strip trailing ':', if any
if r == ':' {
   line = line[0 : len(line)-1]
}

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:311: // contains no punctuation, and special charaters
// exclude lines with illegal characters

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:316: // allow ' as possessive 's only
s/as/for/

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:320: if i == -1 {
if i < 0 {
   break
}

this doesn't rely on a specific (-1) result value but only on the fact that
there is no legal (>= 0) index

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:323: if i == len(b)-1 {
if i+1 >= len(b) || b[i+1] != 's' || (i+2 < len(b) && b[i+2] != ' ') {
    return nil // not followed by "'s"
}

and remove the next 2 if's

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:411: if lastWasBlank && !lastNonblankWasHeading &&
len(lines) >= i+2 &&
if lastWasBlank && !lastNonblankWasHeading && i+2 < len(lines) &&
isBlank(lines[i+1] && !isBlank(lines[i+1]) && indentLen(lines[i+2]) == 0 {
  // ...

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:415: // and the next non-blank lines isn't indented:
this
s/lines/line/
s/isn't/is not/

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:425: } else {
}

and remove the else part

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:441: // TODO: remove in prod
just remove this

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment_test.go
File src/pkg/go/doc/comment_test.go (right):

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment_test....
src/pkg/go/doc/comment_test.go:11: var isHeadingTests = []struct {
s/isHeadingTests/tests/

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment_test....
src/pkg/go/doc/comment_test.go:12: in  string
s/in/line/

(match use in actual code)

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment_test....
src/pkg/go/doc/comment_test.go:13: out bool
s/out/ok/

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment_test....
src/pkg/go/doc/comment_test.go:22: {"δ is Greek", false},
This should probably be legal, but it's tricky. Add a comment next to line:

// TODO: consider allowing this

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment_test....
src/pkg/go/doc/comment_test.go:25: {"Fermat's", true},
add a test with "'sX"

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment_test....
src/pkg/go/doc/comment_test.go:34: if h := heading([]byte(tt.in)); (h == nil) ==
tt.out {
I would write:

(h != nil) != tt.ok

since the emphasis is on it not being ok

[gri, 2011-11-30 20:28:12]

PS: Please also update the CL description (remove the paragraph "Review Note").
- gri

On Wed, Nov 30, 2011 at 12:13 PM,  <gri@golang.org> wrote:
> Much better. This heuristic can be relatively easily refined as
> necessary.
>
> Andrew's headscan file is missing. Regarding testing: We don't want to
> have to update a file with rejected or accepted headings when new
> sources are added ot existing sources are modified. Incorrect formatting
> is not breaking the build, and we will just fix it when we see it. The
> headscan tool is useful to test tweaks to the algorithm though, and we
> want to have it available.
>
> Some nitpicks and some suggestions for simplifications.
>
>
> http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/Makefile
> File src/pkg/go/doc/Makefile (right):
>
>
http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/Makefile#newc...
> src/pkg/go/doc/Makefile:19: headscan.$O: headscan.go
> headscan.go is missing from this CL
>
> http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go
> File src/pkg/go/doc/comment.go (right):
>
>
http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
> src/pkg/go/doc/comment.go:276: // Check if line passes the heuristics
> for a valid heading.
> just:
>
> // heading returns the (possibly trimmed) line if it passes as a valid
> section heading;
> // otherwise it returns nil.
>
> Leave the rest of the comment away: 1) there is detailed comments on
> these requirements in the code, 2) this is not an exported functions,
> and 3) the heuristic may change over time and this comment is likely to
> get out of sync.
>
> I would also leave the TODO away. Over time we will see if adjustments
> are needed and we will just make them.
>
>
http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
> src/pkg/go/doc/comment.go:296: // starts with uppercase letter
> // a heading must start with an uppercase letter
>
>
http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
> src/pkg/go/doc/comment.go:302: // ends in a letter or digit
> // it must end in a letter, digit, or ':'
>
>
http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
> src/pkg/go/doc/comment.go:308: // strip trailing : (which is allowed)
> more direct and faster, since we have r already:
>
> // strip trailing ':', if any
> if r == ':' {
>   line = line[0 : len(line)-1]
> }
>
>
http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
> src/pkg/go/doc/comment.go:311: // contains no punctuation, and special
> charaters
> // exclude lines with illegal characters
>
>
http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
> src/pkg/go/doc/comment.go:316: // allow ' as possessive 's only
> s/as/for/
>
>
http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
> src/pkg/go/doc/comment.go:320: if i == -1 {
> if i < 0 {
>   break
> }
>
> this doesn't rely on a specific (-1) result value but only on the fact
> that there is no legal (>= 0) index
>
>
http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
> src/pkg/go/doc/comment.go:323: if i == len(b)-1 {
> if i+1 >= len(b) || b[i+1] != 's' || (i+2 < len(b) && b[i+2] != ' ') {
>    return nil // not followed by "'s"
> }
>
> and remove the next 2 if's
>
>
http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
> src/pkg/go/doc/comment.go:411: if lastWasBlank &&
> !lastNonblankWasHeading && len(lines) >= i+2 &&
> if lastWasBlank && !lastNonblankWasHeading && i+2 < len(lines) &&
> isBlank(lines[i+1] && !isBlank(lines[i+1]) && indentLen(lines[i+2]) == 0
> {
>  // ...
>
>
http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
> src/pkg/go/doc/comment.go:415: // and the next non-blank lines isn't
> indented: this
> s/lines/line/
> s/isn't/is not/
>
>
http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
> src/pkg/go/doc/comment.go:425: } else {
> }
>
> and remove the else part
>
>
http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
> src/pkg/go/doc/comment.go:441: // TODO: remove in prod
> just remove this
>
>
http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment_test.go
> File src/pkg/go/doc/comment_test.go (right):
>
>
http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment_test....
> src/pkg/go/doc/comment_test.go:11: var isHeadingTests = []struct {
> s/isHeadingTests/tests/
>
>
http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment_test....
> src/pkg/go/doc/comment_test.go:12: in  string
> s/in/line/
>
> (match use in actual code)
>
>
http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment_test....
> src/pkg/go/doc/comment_test.go:13: out bool
> s/out/ok/
>
>
http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment_test....
> src/pkg/go/doc/comment_test.go:22: {"δ is Greek", false},
> This should probably be legal, but it's tricky. Add a comment next to
> line:
>
> // TODO: consider allowing this
>
>
http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment_test....
> src/pkg/go/doc/comment_test.go:25: {"Fermat's", true},
> add a test with "'sX"
>
>
http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment_test....
> src/pkg/go/doc/comment_test.go:34: if h := heading([]byte(tt.in)); (h ==
> nil) == tt.out {
> I would write:
>
> (h != nil) != tt.ok
>
> since the emphasis is on it not being ok
>
> http://codereview.appspot.com/5437056/
>

[volker.dobler, 2011-11-30 22:42:35]

I was wondering wether src/pkg/gp/doc was the right
place for the headscan tool.  But it seems as if there
are other executables located in the src/pkg tree.

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go
File src/pkg/go/doc/comment.go (right):

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:276: // Check if line passes the heuristics for a
valid heading.
On 2011/11/30 20:13:54, gri wrote:
> just:
> 
> // heading returns the (possibly trimmed) line if it passes as a valid section
> heading;
> // otherwise it returns nil.
> 
> Leave the rest of the comment away: 1) there is detailed comments on these
> requirements in the code, 2) this is not an exported functions, and 3) the
> heuristic may change over time and this comment is likely to get out of sync.
> 
> I would also leave the TODO away. Over time we will see if adjustments are
> needed and we will just make them.

Done.

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:296: // starts with uppercase letter
On 2011/11/30 20:13:54, gri wrote:
> // a heading must start with an uppercase letter

Done.

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:302: // ends in a letter or digit
On 2011/11/30 20:13:54, gri wrote:
> // it must end in a letter, digit, or ':'

Done.

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:308: // strip trailing : (which is allowed)
On 2011/11/30 20:13:54, gri wrote:
> more direct and faster, since we have r already:
> 
> // strip trailing ':', if any
> if r == ':' {
>    line = line[0 : len(line)-1]
> }

Done.

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:311: // contains no punctuation, and special charaters
On 2011/11/30 20:13:54, gri wrote:
> // exclude lines with illegal characters

Done.

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:316: // allow ' as possessive 's only
On 2011/11/30 20:13:54, gri wrote:
> s/as/for/

Done.

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:320: if i == -1 {
On 2011/11/30 20:13:54, gri wrote:
> if i < 0 {
>    break
> }
> 
> this doesn't rely on a specific (-1) result value but only on the fact that
> there is no legal (>= 0) index

Done.

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:323: if i == len(b)-1 {
On 2011/11/30 20:13:54, gri wrote:
> if i+1 >= len(b) || b[i+1] != 's' || (i+2 < len(b) && b[i+2] != ' ') {
>     return nil // not followed by "'s"
> }
> 
> and remove the next 2 if's

Done.

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:411: if lastWasBlank && !lastNonblankWasHeading &&
len(lines) >= i+2 &&
On 2011/11/30 20:13:54, gri wrote:
> if lastWasBlank && !lastNonblankWasHeading && i+2 < len(lines) &&
> isBlank(lines[i+1] && !isBlank(lines[i+1]) && indentLen(lines[i+2]) == 0 {
>   // ...

Done.

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:415: // and the next non-blank lines isn't indented:
this
On 2011/11/30 20:13:54, gri wrote:
> s/lines/line/
> s/isn't/is not/

Done.

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:425: } else {
On 2011/11/30 20:13:54, gri wrote:
> }
> 
> and remove the else part

Done.

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment.go#ne...
src/pkg/go/doc/comment.go:441: // TODO: remove in prod
On 2011/11/30 20:13:54, gri wrote:
> just remove this

Done.

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment_test.go
File src/pkg/go/doc/comment_test.go (right):

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment_test....
src/pkg/go/doc/comment_test.go:11: var isHeadingTests = []struct {
On 2011/11/30 20:13:54, gri wrote:
> s/isHeadingTests/tests/

Mmmh. "isHeadingTest" as well as "in" and "out" where
suggestions made by Russ. 
See
http://codereview.appspot.com/5437056/diff/11004/src/pkg/go/doc/comment_test....

I'll s/isHeading/heading/ as this reflects the current
naming.  Andrew asked for tests for ToHTML() which I'd
like to provide (in the near future) so keeping the type
of test seems resonable.

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment_test....
src/pkg/go/doc/comment_test.go:12: in  string
On 2011/11/30 20:13:54, gri wrote:
> s/in/line/
> 
> (match use in actual code)

Done.

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment_test....
src/pkg/go/doc/comment_test.go:13: out bool
On 2011/11/30 20:13:54, gri wrote:
> s/out/ok/

Done. Especially as it is no longer the output of 
heading() itself.

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment_test....
src/pkg/go/doc/comment_test.go:22: {"δ is Greek", false},
On 2011/11/30 20:13:54, gri wrote:
> This should probably be legal, but it's tricky. Add a comment next to line:
> 
> // TODO: consider allowing this
Done. Just to know: Why should this be legal? Just
because it is Greek?  A German heading may not start
with ä also...

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment_test....
src/pkg/go/doc/comment_test.go:25: {"Fermat's", true},
On 2011/11/30 20:13:54, gri wrote:
> add a test with "'sX"

Done.

http://codereview.appspot.com/5437056/diff/15001/src/pkg/go/doc/comment_test....
src/pkg/go/doc/comment_test.go:34: if h := heading([]byte(tt.in)); (h == nil) ==
tt.out {
On 2011/11/30 20:13:54, gri wrote:
> I would write:
> 
> (h != nil) != tt.ok
> 
> since the emphasis is on it not being ok

Done.

[volker.dobler, 2011-11-30 22:49:37]

Hello jan.mercl@nic.cz, gri@golang.org, adg@golang.org, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[volker.dobler, 2011-11-30 22:50:26]

Hello jan.mercl@nic.cz, gri@golang.org, adg@golang.org, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[adg, 2011-11-30 23:06:46]

http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/headscan.go
File src/pkg/go/doc/headscan.go (right):

http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/headscan.go#newco...
src/pkg/go/doc/headscan.go:39: i := bytes.Index(b, []byte("<h3>"))
If you use <h4> now then this won't work anymore.

[gri, 2011-11-30 23:12:45]

On Wed, Nov 30, 2011 at 3:06 PM,  <adg@golang.org> wrote:
>
http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/headscan.go#newco...
> src/pkg/go/doc/headscan.go:39: i := bytes.Index(b, []byte("<h3>"))
> If you use <h4> now then this won't work anymore.

it's <h3> still - should be fine
- gri

[gri, 2011-11-30 23:24:23]

Almost there. Some minor issues and a bug.
- gri

http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/comment.go
File src/pkg/go/doc/comment.go (right):

http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/comment.go#newcod...
src/pkg/go/doc/comment.go:276: // eading returns the (possibly trimmed) line if
it passes as a valid section
s/eading/heading/

http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/comment.go#newcod...
src/pkg/go/doc/comment.go:396: isBlank(lines[i+1]) && !isBlank(lines[i+1]) &&
indentLen(lines[i+2]) == 0 {
this should be 

!isBlank(lines[i+2])

otherwise it won't work

http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/comment_test.go
File src/pkg/go/doc/comment_test.go (right):

http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/comment_test.go#n...
src/pkg/go/doc/comment_test.go:22: {"δ is Greek", false}, // TODO: consider
allowing this
To answer your question.

Not sure we should allow it, but lower-case greek letters can make sense. E.g.,
I can imagine a title (making something up here):

δ-distribution flags

bla bla bla ...

(i.e., a technical term may use a lower-case Greek letter and just making it
upper-case may make the technical term unrecognizable). But it's speculation,
which is why there's just a TODO.

http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/headscan.go
File src/pkg/go/doc/headscan.go (right):

http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/headscan.go#newco...
src/pkg/go/doc/headscan.go:39: i := bytes.Index(b, []byte("<h3>"))
On 2011/11/30 23:06:46, adg wrote:
> If you use <h4> now then this won't work anymore.

Done.

http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/headscan.go#newco...
src/pkg/go/doc/headscan.go:53: /*
remove this code - it's dead

[gri, 2011-11-30 23:26:33]

Also: Please fix headscan.go - it doesn't compile anymore (still uses
the old-style os.FileInfo).
- gri

On Wed, Nov 30, 2011 at 3:24 PM,  <gri@golang.org> wrote:
> Almost there. Some minor issues and a bug.
> - gri
>
>
> http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/comment.go
> File src/pkg/go/doc/comment.go (right):
>
>
http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/comment.go#newcod...
> src/pkg/go/doc/comment.go:276: // eading returns the (possibly trimmed)
> line if it passes as a valid section
> s/eading/heading/
>
>
http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/comment.go#newcod...
> src/pkg/go/doc/comment.go:396: isBlank(lines[i+1]) &&
> !isBlank(lines[i+1]) && indentLen(lines[i+2]) == 0 {
> this should be
>
> !isBlank(lines[i+2])
>
> otherwise it won't work
>
> http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/comment_test.go
> File src/pkg/go/doc/comment_test.go (right):
>
>
http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/comment_test.go#n...
> src/pkg/go/doc/comment_test.go:22: {"δ is Greek", false}, // TODO:
> consider allowing this
> To answer your question.
>
> Not sure we should allow it, but lower-case greek letters can make
> sense. E.g., I can imagine a title (making something up here):
>
> δ-distribution flags
>
> bla bla bla ...
>
> (i.e., a technical term may use a lower-case Greek letter and just
> making it upper-case may make the technical term unrecognizable). But
> it's speculation, which is why there's just a TODO.
>
> http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/headscan.go
> File src/pkg/go/doc/headscan.go (right):
>
>
http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/headscan.go#newco...
> src/pkg/go/doc/headscan.go:39: i := bytes.Index(b, []byte("<h3>"))
> On 2011/11/30 23:06:46, adg wrote:
>>
>> If you use <h4> now then this won't work anymore.
>
> Done.
>
>
http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/headscan.go#newco...
> src/pkg/go/doc/headscan.go:53: /*
> remove this code - it's dead
>
> http://codereview.appspot.com/5437056/
>

[jnml, 2011-12-01 08:32:34]

On 2011/11/30 23:24:23, gri wrote:
> Almost there. Some minor issues and a bug.

Is there any chance to know Rob's stance on this CL?

I'm disappointed by the fact that the proposal was rejected in the discussion in
the mailing list by development team members involved - but the CL was "pushed"
anyway regardless their verdict.

I share the opposing opinions about this idea as cited earlier in this thread.

-jnml

[volker.dobler, 2011-12-01 09:03:41]

Hello jan.mercl@nic.cz, gri@golang.org, adg@golang.org, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[volker.dobler, 2011-12-01 09:03:49]

http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/comment.go
File src/pkg/go/doc/comment.go (right):

http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/comment.go#newcod...
src/pkg/go/doc/comment.go:276: // eading returns the (possibly trimmed) line if
it passes as a valid section
On 2011/11/30 23:24:23, gri wrote:
> s/eading/heading/

Done.

http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/comment.go#newcod...
src/pkg/go/doc/comment.go:396: isBlank(lines[i+1]) && !isBlank(lines[i+1]) &&
indentLen(lines[i+2]) == 0 {
On 2011/11/30 23:24:23, gri wrote:
> this should be 
> 
> !isBlank(lines[i+2])
> 
> otherwise it won't work

Done.

http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/comment_test.go
File src/pkg/go/doc/comment_test.go (right):

http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/comment_test.go#n...
src/pkg/go/doc/comment_test.go:22: {"δ is Greek", false}, // TODO: consider
allowing this
On 2011/11/30 23:24:23, gri wrote:
> To answer your question.
> 
> Not sure we should allow it, but lower-case greek letters can make sense.
E.g.,
> I can imagine a title (making something up here):
> 
> δ-distribution flags
> 
> bla bla bla ...
> 
> (i.e., a technical term may use a lower-case Greek letter and just making it
> upper-case may make the technical term unrecognizable). But it's speculation,
> which is why there's just a TODO.

I understand. Personaly I would require a reformulation
of the heading in the sens of: Just dont' write
  δ-function
but
  The δ-function

http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/headscan.go
File src/pkg/go/doc/headscan.go (right):

http://codereview.appspot.com/5437056/diff/6/src/pkg/go/doc/headscan.go#newco...
src/pkg/go/doc/headscan.go:53: /*
On 2011/11/30 23:24:23, gri wrote:
> remove this code - it's dead

Done.

[volker.dobler, 2011-12-01 09:27:04]

On Thu, Dec 1, 2011 at 9:32 AM, <jan.mercl@nic.cz> wrote:

>
> Is there any chance to know Rob's stance on this CL?
>
> I'm disappointed by the fact that the proposal was rejected in the
> discussion in the mailing list by development team members involved -
> but the CL was "pushed" anyway regardless their verdict.
>
> I share the opposing opinions about this idea as cited earlier in this
> thread.
>
>
I am not sure if the proposal itself was rejected. But you are
right that there was a controversial discussion whether detection
of headings is worth the effort, can be done without markup,
would distract from writing good docs, solves a problem that
does not exist or simply is not needed.

As I understood the discussion (being biased of course):
 - Lots of people are missing headings in longer doc.
 - Almost everyone was afraid that it couldn't be done
   properly without additional markup.

This CL was to explore what can and cannot be done in this
area, how complex the solution would be and check the impact
on existing documentation.

IMHO the current state is:
 - Headings detection works reliable on existing code
   i.e. no false positives and no false negatives.
 - The complexity (in code and runtime) is low.
 - Additional markup is not needed.

It is up to Russ, Andrew, Robert or Rob to decide how to proceed.

Volker

http://codereview.appspot.com/**5437056/<http://codereview.appspot.com/5437056/>
>

[r2, 2011-12-01 16:54:17]

On Dec 1, 2011, at 12:32 AM, jan.mercl@nic.cz wrote:

> On 2011/11/30 23:24:23, gri wrote:
>> Almost there. Some minor issues and a bug.
> 
> Is there any chance to know Rob's stance on this CL?

Gri and I had a long talk about it and agreed that if a design comes that
doesn't break existing comments and doesn't require extra formatting and doesn't
encourage more formatting suggestions, we'd consider it.

-rob

[rsc, 2011-12-01 17:01:06]

Just to add to what Rob said, the key point about Go comments
in contrast to Javadoc or Pydoc or whatever is that the goal is
that they look like comments, not markup.  It is okay to take
advantage of conventions that already exist as hints to how to
make them HTML, but not okay to force new conventions.
I was skeptical of this CL but it really does seem to be converging
on the former, which is great.

Russ

[gri, 2011-12-01 17:04:42]

Agreed. I think the current CL is close: It just does the right thing
w/o the comment author having to know extra rules.
- gri

On Thu, Dec 1, 2011 at 9:01 AM, Russ Cox <rsc@golang.org> wrote:
> Just to add to what Rob said, the key point about Go comments
> in contrast to Javadoc or Pydoc or whatever is that the goal is
> that they look like comments, not markup.  It is okay to take
> advantage of conventions that already exist as hints to how to
> make them HTML, but not okay to force new conventions.
> I was skeptical of this CL but it really does seem to be converging
> on the former, which is great.
>
> Russ
>

[jnml, 2011-12-01 17:05:21]

On 2011/12/01 16:54:17, r2 wrote:
> Gri and I had a long talk about it and agreed that if a design comes that
> doesn't break existing comments and doesn't require extra formatting and
doesn't
> encourage more formatting suggestions, we'd consider it.

Thanks for letting me know. Full respect.

-jnml

[gri, 2011-12-01 17:45:18]

LGTM.
Thanks for bearing w/ me.
- gri

[gri, 2011-12-01 17:50:01]

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

go/doc: Detect headings in comments and format them as h3 in html.

To structure larger sections of comments in html output headings
are detected in comments and formated as h3 in the generated html.
A simple heuristic is used to detect headings in comments:
A heading is a non-blank, non-indented line preceded by a blank
line. It is followed by a blank and a non-blank, non-indented line.
A heading must start with an uppercase letter and end with a letter,
digit or a colon.  A heading may not contain punctuation characters.

R=jan.mercl, gri, adg, rsc, r
CC=golang-dev
http://codereview.appspot.com/5437056

Committer: Robert Griesemer <gri@golang.org>