code review 6935071: go/doc: add "hdr-" prefix to headers generated from pac...

go/doc: add "hdr-" prefix to headers generated from package overviews.

[dsymonds, 2012-12-18 00:33:51]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2012-12-18 00:55:53]

To which ones? Where's the conflict?

On Mon, Dec 17, 2012 at 4:33 PM, <dsymonds@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> go/doc: add "hdr-" prefix to headers generated from package overviews.
>
> Please review this at
https://codereview.appspot.**com/6935071/<https://codereview.appspot.com/6935...
>
> Affected files:
>   M src/pkg/go/doc/comment.go
>
>
> Index: src/pkg/go/doc/comment.go
> ==============================**==============================**=======
> --- a/src/pkg/go/doc/comment.go
> +++ b/src/pkg/go/doc/comment.go
> @@ -229,7 +229,8 @@
>  var nonAlphaNumRx = regexp.MustCompile(`[^a-zA-Z0-**9]`)
>
>  func anchorID(line string) string {
> -       return nonAlphaNumRx.**ReplaceAllString(line, "_")
> +       // Add a "hdr-" prefix to avoid conflicting with IDs used for
> package symbols.
> +       return "hdr-" + nonAlphaNumRx.**ReplaceAllString(line, "_")
>  }
>
>  // ToHTML converts comment text to formatted HTML.
>
>
>

[bradfitz, 2012-12-18 00:59:56]

LGTM

Oh, just those <h3s> in the package overview.


On Mon, Dec 17, 2012 at 4:55 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> To which ones? Where's the conflict?
>
>
> On Mon, Dec 17, 2012 at 4:33 PM, <dsymonds@golang.org> wrote:
>
>> Reviewers: golang-dev_googlegroups.com,
>>
>> Message:
>> Hello golang-dev@googlegroups.com,
>>
>> I'd like you to review this change to
>> https://go.googlecode.com/hg/
>>
>>
>> Description:
>> go/doc: add "hdr-" prefix to headers generated from package overviews.
>>
>> Please review this at
https://codereview.appspot.**com/6935071/<https://codereview.appspot.com/6935...
>>
>> Affected files:
>>   M src/pkg/go/doc/comment.go
>>
>>
>> Index: src/pkg/go/doc/comment.go
>> ==============================**==============================**=======
>> --- a/src/pkg/go/doc/comment.go
>> +++ b/src/pkg/go/doc/comment.go
>> @@ -229,7 +229,8 @@
>>  var nonAlphaNumRx = regexp.MustCompile(`[^a-zA-Z0-**9]`)
>>
>>  func anchorID(line string) string {
>> -       return nonAlphaNumRx.**ReplaceAllString(line, "_")
>> +       // Add a "hdr-" prefix to avoid conflicting with IDs used for
>> package symbols.
>> +       return "hdr-" + nonAlphaNumRx.**ReplaceAllString(line, "_")
>>  }
>>
>>  // ToHTML converts comment text to formatted HTML.
>>
>>
>>
>

[gri, 2012-12-18 01:10:45]

LGTM

[dsymonds, 2012-12-18 01:19:15]

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

go/doc: add "hdr-" prefix to headers generated from package overviews.

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

[adg, 2013-01-15 05:48:10]

This has broken anyone who ever linked to a fragment. There are many in our
codebase, eg doc/code.html.

:-(

[dsymonds, 2013-01-15 05:54:57]

On Tue, Jan 15, 2013 at 4:47 PM, Andrew Gerrand <adg@golang.org> wrote:

> This has broken anyone who ever linked to a fragment. There are many in our
> codebase, eg doc/code.html.
>
> :-(

At least one such breakage has already been fixed. We've just got to
go fix the others.

[gburd, 2013-01-15 13:16:08]

On Monday, January 14, 2013 9:47:39 PM UTC-8, Andrew Gerrand wrote:

> This has broken anyone who ever linked to a fragment. There are many in 
> our codebase, eg doc/code.html.
>
> :-(
>

This change also brok links to godoc.org.

[minux1, 2013-01-15 14:03:29]

On Tue, Jan 15, 2013 at 9:16 PM, Gary Burd <gary.burd@gmail.com> wrote:

> On Monday, January 14, 2013 9:47:39 PM UTC-8, Andrew Gerrand wrote:
>
>> This has broken anyone who ever linked to a fragment. There are many in
>> our codebase, eg doc/code.html.
>>  :-(
>>
>
> This change also brok links to godoc.org.
>
perhaps we'd better revert this change.

[adg, 2013-01-15 21:14:50]

On 16 January 2013 00:16, Gary Burd <gary.burd@gmail.com> wrote:

> This change also brok links to godoc.org.
>

I'm surprised.
Links to or links from?
Can you provide an example?

[minux1, 2013-01-15 21:18:02]

On Wed, Jan 16, 2013 at 5:14 AM, Andrew Gerrand <adg@golang.org> wrote:
>
> On 16 January 2013 00:16, Gary Burd <gary.burd@gmail.com> wrote:
>
>> This change also brok links to godoc.org.
>>
>
> I'm surprised.
> Links to or links from?
> Can you provide an example?
>
I think he means godoc.org also uses go/doc package, so this change also
affects links to sections on godoc.org

[gburd, 2013-01-15 21:49:55]

> Links to or links from?
> Can you provide an example?
>

From sites that link to godoc.org.  Example: 
https://github.com/garyburd/redigo#features 

[minux1, 2013-01-15 21:57:54]

On Wed, Jan 16, 2013 at 5:49 AM, Gary Burd <gary.burd@gmail.com> wrote:

>
> Links to or links from?
>> Can you provide an example?
>>
>
> From sites that link to godoc.org.  Example:
> https://github.com/garyburd/redigo#features
>
I wonder if this should be covered by the Go 1 API contract.

I believe so because this is go/doc's external behavior. I'd suggest we
revert this change and
add a note or BUG perhaps (and arguably this id clash doesn't happen pretty
often).

[adg, 2013-01-15 22:11:42]

On 16 January 2013 08:57, minux <minux.ma@gmail.com> wrote:

> I wonder if this should be covered by the Go 1 API contract.


Not technically but in spirit perhaps.

Dave, what was the original motivation for this change? I find the #hdr-
prefix kinda gross anyway. I think I'd prefer the occasional conflict.

[dsymonds, 2013-01-15 22:21:20]

The original motivation was a large package that had an overview section
header with the same name as a type in the package. The conflict meant that
the index was useless for getting to that type; worse, it was confusing
when the page jumped to the completely wrong place.

I don't want to roll this back. It's a one-time small pain to do the right
thing, and avoids conflicts that can be introduced with an innocuous change
to some package docs. I would think that preserving existing links would be
less important than making sure the index links always work.

[gburd, 2013-01-15 22:22:02]

It's difficult to find out how many links are affected. It's possible that 
the links in my readme file and the links mentioned earlier are the only 
ones.

Because two comments can include the same header text, this change does not 
eliminate all possible id conflicts.

[adg, 2013-01-15 22:26:51]

On 16 January 2013 09:22, Gary Burd <gary.burd@gmail.com> wrote:

> Because two comments can include the same header text, this change does
> not eliminate all possible id conflicts.


That is very unlikely compared to the motivating example. For one thing, we
haven't seen it yet.

[minux1, 2013-01-15 22:28:28]

On Wed, Jan 16, 2013 at 6:21 AM, David Symonds <dsymonds@golang.org> wrote:

> The original motivation was a large package that had an overview section
> header with the same name as a type in the package. The conflict meant that
> the index was useless for getting to that type; worse, it was confusing
> when the page jumped to the completely wrong place.
>
How about we do this: we use javascript to update real conflicting ids, so
that
if you've got two ids with the same name, you have to use uglier fragment to
access them, and so we can minimise the impact of a large collections of
existing
packages.

[adg, 2013-01-15 22:30:47]

On 16 January 2013 09:28, minux <minux.ma@gmail.com> wrote:

> How about we do this: we use javascript to update real conflicting ids, so
> that
> if you've got two ids with the same name, you have to use uglier fragment
> to
> access them, and so we can minimise the impact of a large collections of
> existing
> packages.
>

IMO the ideal amount of JS used to render a doc page is none.

Honestly I'd prefer to add some gross hack to godoc than rewrite with JS.

[minux1, 2013-01-15 22:32:51]

On Wed, Jan 16, 2013 at 6:30 AM, Andrew Gerrand <adg@golang.org> wrote:
>
> On 16 January 2013 09:28, minux <minux.ma@gmail.com> wrote:
>
>> How about we do this: we use javascript to update real conflicting ids,
>> so that
>> if you've got two ids with the same name, you have to use uglier fragment
>> to
>> access them, and so we can minimise the impact of a large collections of
>> existing
>> packages.
>>
>
> IMO the ideal amount of JS used to render a doc page is none.
> Honestly I'd prefer to add some gross hack to godoc than rewrite with JS.
>
Sure, we can also do the same in cmd/godoc (or go/doc).

[bradfitz, 2013-01-15 23:53:47]

On Tue, Jan 15, 2013 at 2:30 PM, Andrew Gerrand <adg@golang.org> wrote:

>
> On 16 January 2013 09:28, minux <minux.ma@gmail.com> wrote:
>
>> How about we do this: we use javascript to update real conflicting ids,
>> so that
>> if you've got two ids with the same name, you have to use uglier fragment
>> to
>> access them, and so we can minimise the impact of a large collections of
>> existing
>> packages.
>>
>
> IMO the ideal amount of JS used to render a doc page is none.
>
> Honestly I'd prefer to add some gross hack to godoc than rewrite with JS.
>

I wouldn't rewrite with JS, and I'd keep the HTML as it is now, but you
could add JS such that if the page is #Foo and
document.getElementById("Foo") doesn't exist, but "hdr-Foo", does, change
document.location.hash to "hdr-Foo".

Then we don't break any old links.

[dsymonds, 2013-01-16 00:08:26]

On Wed, Jan 16, 2013 at 10:53 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> I wouldn't rewrite with JS, and I'd keep the HTML as it is now, but you
> could add JS such that if the page is #Foo and
> document.getElementById("Foo") doesn't exist, but "hdr-Foo", does, change
> document.location.hash to "hdr-Foo".
>
> Then we don't break any old links.

I would be fine with this (though I don't think it's necessary).

[adg, 2013-01-16 00:12:11]

On 16 January 2013 11:08, David Symonds <dsymonds@golang.org> wrote:

> I would be fine with this (though I don't think it's necessary).


At least it provides an option for Gary on godoc.org.

I would prefer not to revert this change, either.

[gburd, 2013-01-16 02:43:55]

> That is very unlikely compared to the motivating example. For one thing, 
we haven't seen it yet.

Here's an example where two comments use the same header text: 
http://godoc.org/code.google.com/p/rsc/fuse

[adg, 2013-01-16 03:12:15]

Actually, it's three in that case!

It's worth noting that this change doesn't affect that issue.