Grokbase › Groups › Go › golang-dev › December 2012
FAQ
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/

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.

Search Discussions

  • Brad Fitzpatrick at Dec 18, 2012 at 12:55 am ⇧
    To which ones? Where's the conflict?
    On Mon, Dec 17, 2012 at 4:33 PM, 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/6935071/>

    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.

  • Brad Fitzpatrick at Dec 18, 2012 at 12:59 am ⇧
    LGTM

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

    On Mon, Dec 17, 2012 at 4:55 PM, Brad Fitzpatrick wrote:

    To which ones? Where's the conflict?

    On Mon, Dec 17, 2012 at 4:33 PM, 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/6935071/>

    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 at Dec 18, 2012 at 1:10 am ⇧
    LGTM

    https://codereview.appspot.com/6935071/
  • Dsymonds at Dec 18, 2012 at 1:19 am ⇧
    *** 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


    https://codereview.appspot.com/6935071/
  • Andrew Gerrand at Jan 15, 2013 at 5:48 am ⇧
    This has broken anyone who ever linked to a fragment. There are many in our
    codebase, eg doc/code.html.

    :-(
  • David Symonds at Jan 15, 2013 at 5:54 am ⇧

    On Tue, Jan 15, 2013 at 4:47 PM, Andrew Gerrand 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.
  • Gary Burd at Jan 15, 2013 at 1:16 pm ⇧

    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.
  • Minux at Jan 15, 2013 at 2:03 pm ⇧

    On Tue, Jan 15, 2013 at 9:16 PM, Gary Burd 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.
  • Andrew Gerrand at Jan 15, 2013 at 9:14 pm ⇧

    On 16 January 2013 00:16, Gary Burd wrote:

    This change also brok links to godoc.org.
    I'm surprised.
    Links to or links from?
    Can you provide an example?
  • Minux at Jan 15, 2013 at 9:18 pm ⇧

    On Wed, Jan 16, 2013 at 5:14 AM, Andrew Gerrand wrote:
    On 16 January 2013 00:16, Gary Burd 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
  • Gary Burd at Jan 15, 2013 at 9:49 pm ⇧

    Links to or links from?
    Can you provide an example?
    From sites that link to godoc.org. Example:
    https://github.com/garyburd/redigo#features
  • Minux at Jan 15, 2013 at 9:57 pm ⇧

    On Wed, Jan 16, 2013 at 5:49 AM, Gary Burd 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).
  • Andrew Gerrand at Jan 15, 2013 at 10:11 pm ⇧

    On 16 January 2013 08:57, minux 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.
  • David Symonds at Jan 15, 2013 at 10:21 pm ⇧
    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.
  • Minux at Jan 15, 2013 at 10:28 pm ⇧

    On Wed, Jan 16, 2013 at 6:21 AM, David Symonds 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.
  • Andrew Gerrand at Jan 15, 2013 at 10:38 pm ⇧

    On 16 January 2013 09:28, minux 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.
  • Minux at Jan 15, 2013 at 10:32 pm ⇧

    On Wed, Jan 16, 2013 at 6:30 AM, Andrew Gerrand wrote:
    On 16 January 2013 09:28, minux 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).
  • Brad Fitzpatrick at Jan 15, 2013 at 11:53 pm ⇧
    On Tue, Jan 15, 2013 at 2:30 PM, Andrew Gerrand wrote:
    On 16 January 2013 09:28, minux 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.
  • David Symonds at Jan 16, 2013 at 12:08 am ⇧

    On Wed, Jan 16, 2013 at 10:53 AM, Brad Fitzpatrick 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).
  • Andrew Gerrand at Jan 16, 2013 at 12:12 am ⇧

    On 16 January 2013 11:08, David Symonds 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.
  • Gary Burd at Jan 15, 2013 at 10:22 pm ⇧
    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.
  • Andrew Gerrand at Jan 15, 2013 at 10:26 pm ⇧

    On 16 January 2013 09:22, Gary Burd 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.
  • Gary Burd at Jan 16, 2013 at 2:43 am ⇧
    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
  • Andrew Gerrand at Jan 16, 2013 at 3:12 am ⇧
    Actually, it's three in that case!

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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedDec 18, '12 at 12:33a
activeJan 16, '13 at 3:12a
posts25
users6
websitegolang.org

6 users in discussion

Andrew Gerrand: 7 posts Minux: 5 posts David Symonds: 5 posts Gary Burd: 4 posts Brad Fitzpatrick: 3 posts Gri: 1 post

Content

People

Support

Translate

site design / logo © 2022 Grokbase