code review 5616060: go/doc: Fix URL linking in ToHTML.

go/doc: Fix URL linking in ToHTML.

Fixes issue: 2832

[gburd, 2012-02-02 20:19:07]

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

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

[rsc, 2012-02-02 22:43:45]

test looks good; not sure about the comment

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

http://codereview.appspot.com/5616060/diff/4001/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:103: // match against first parenthesized sub-regexp;
must be match against urlRx
But urlRx is now m[1]?

[gri, 2012-02-02 22:44:57]

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

http://codereview.appspot.com/5616060/diff/4001/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:90: // m >= 6 (two parenthesized sub-regexps in
matchRx, 1st one is identRx)
1st one is urlRx

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

http://codereview.appspot.com/5616060/diff/4001/src/pkg/go/doc/comment_test.g...
src/pkg/go/doc/comment_test.go:90: {"http://www.google.com/", "<a
href=\"http://www.google.com/\">http://www.google.com/</a>"},
use back-quoted (raw) strings, then there is no need for \"

http://codereview.appspot.com/5616060/diff/4001/src/pkg/go/doc/comment_test.g...
src/pkg/go/doc/comment_test.go:90: {"http://www.google.com/", "<a
href=\"http://www.google.com/\">http://www.google.com/</a>"},
should add a few more test cases, exercising the regexp

[gburd, 2012-02-02 23:56:44]

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

Please take another look.

[gburd, 2012-02-03 00:00:20]

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

http://codereview.appspot.com/5616060/diff/4001/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:90: // m >= 6 (two parenthesized sub-regexps in
matchRx, 1st one is identRx)
On 2012/02/02 22:44:57, gri wrote:
> 1st one is urlRx

Fixed.

http://codereview.appspot.com/5616060/diff/4001/src/pkg/go/doc/comment.go#new...
src/pkg/go/doc/comment.go:103: // match against first parenthesized sub-regexp;
must be match against urlRx
On 2012/02/02 22:43:45, rsc wrote:
> But urlRx is now m[1]?

m[1] is the end of the entire match.  urlRx is m[2]:m[3].

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

http://codereview.appspot.com/5616060/diff/4001/src/pkg/go/doc/comment_test.g...
src/pkg/go/doc/comment_test.go:90: {"http://www.google.com/", "<a
href=\"http://www.google.com/\">http://www.google.com/</a>"},
On 2012/02/02 22:44:57, gri wrote:
> should add a few more test cases, exercising the regexp

Done.

http://codereview.appspot.com/5616060/diff/4001/src/pkg/go/doc/comment_test.g...
src/pkg/go/doc/comment_test.go:90: {"http://www.google.com/", "<a
href=\"http://www.google.com/\">http://www.google.com/</a>"},
On 2012/02/02 22:44:57, gri wrote:
> use back-quoted (raw) strings, then there is no need for \"

Done.

[rsc, 2012-02-03 00:15:57]

On Thu, Feb 2, 2012 at 19:00,  <gary.burd@gmail.com> wrote:
> m[1] is the end of the entire match.  urlRx is m[2]:m[3].

Forgot the *2; sorry for the noise.

[rsc, 2012-02-03 00:16:35]

LGTM

[gri, 2012-02-03 00:31:47]

LGTM but please add one or two tests with a URL in the middle of some line

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

http://codereview.appspot.com/5616060/diff/2002/src/pkg/go/doc/comment_test.g...
src/pkg/go/doc/comment_test.go:90: {"http://www.google.com/", `<a
href="http://www.google.com/">http://www.google.com/</a>`},
Can you please add a couple of tests where a valid URL is inside longer text as
well. Thanks.

say: "foo bar http://www.google.com/ bal"

[rsc, 2012-02-03 00:40:08]

Leaving for gri to submit.

[gburd, 2012-02-03 00:55:21]

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

Please take another look.

[gburd, 2012-02-03 00:55:32]

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

http://codereview.appspot.com/5616060/diff/2002/src/pkg/go/doc/comment_test.g...
src/pkg/go/doc/comment_test.go:90: {"http://www.google.com/", `<a
href="http://www.google.com/">http://www.google.com/</a>`},
On 2012/02/03 00:31:47, gri wrote:
> Can you please add a couple of tests where a valid URL is inside longer text
as
> well. Thanks.
> 
> say: "foo bar http://www.google.com/ bal"

Done.

[gri, 2012-02-03 01:01:53]

LGTM.
thanks.
- gri

[gri, 2012-02-03 01:02:07]

*** Submitted as 1850ba379b84 ***

go/doc: Fix URL linking in ToHTML.

Fixes issue: 2832

R=rsc, gri
CC=golang-dev
http://codereview.appspot.com/5616060

Committer: Robert Griesemer <gri@golang.org>