code review 6847128: go.talks/pkg/present: Adding inline links with style.

go.talks/pkg/present: Adding inline links with style.

Read doc.go for more details.

[francesc, 2012-12-01 00:32:46]

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

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

[adg, 2012-12-01 11:41:20]

https://codereview.appspot.com/6847128/diff/5002/pkg/present/link.go
File pkg/present/link.go (right):

https://codereview.appspot.com/6847128/diff/5002/pkg/present/link.go#newcode50
pkg/present/link.go:50: return template.HTML(fmt.Sprintf(`<a href=%q>%s</a>`,
url.String(), label)), nil
change this to
  return template.HTML(renderLink(url.String(), label)), nil

and add this function:

func renderLink(url, text string) string {
    if text == "" {
        text = url
    }
    return fmt.Sprintf(`<a href="%s" target="_blank">%s</a>`, url, text)
}

https://codereview.appspot.com/6847128/diff/5002/pkg/present/link.go#newcode53
pkg/present/link.go:53: var inlineExp =
regexp.MustCompile(`^\[\[([^\[\]]+)\](\[([^\[\]]*)\])?\]`)
Gross regexp. Please read:
 
http://commandcenter.blogspot.sg/2011/08/regular-expressions-in-lexing-and.html

I think this would be much nicer as a simple function that does everything the
obvious way:

// parseInlineLink parses an inline link at the start of s, and returns
// a rendered HTML link and the total length of the raw inline link.
// If no inline link is present, it returns all zeroes.
func parseInlineLink(s string) (link string, length int) {
	if len(s) < 2 || s[0] != '[' || s[1] != '[' {
		return
	}
	end := strings.Index(s, "]]")
	if end == -1 {
		return
	}
	urlEnd := strings.Index(s, "]")
	if urlEnd == -1 {
		return
	}
	url := s[2:urlEnd]
	const badURLChars = `<>"{}|\^~[]` + "`"	// per RFC1738 section 2.2
	if strings.ContainsAny(url, badURLChars) {
		return
	}
	if urlEnd == end {
		return renderLink(url, ""), end + 2
	}
	if s[urlEnd:urlEnd+2] != "][" {
		return
	}
	text := s[urlEnd+2 : end]
	return renderLink(url, text), end + 2
}

now the other functions are unnecessary.

https://codereview.appspot.com/6847128/diff/5002/pkg/present/style.go
File pkg/present/style.go (right):

https://codereview.appspot.com/6847128/diff/5002/pkg/present/style.go#newcode49
pkg/present/style.go:49: if isInlineLink(word) {
if link, _ := parseInlineLink(word); link != "" {
    words[w] = link
    continue Word
}

https://codereview.appspot.com/6847128/diff/5002/pkg/present/style.go#newcode132
pkg/present/style.go:132: if start, end, found := findInlineLink(s, i); found {
if _, length := parseInlineLink(s[i:]) length > 0 {
  words = append(s[i:i+length])
  mark = i+length
}

https://codereview.appspot.com/6847128/diff/5002/pkg/present/style_test.go
File pkg/present/style_test.go (right):

https://codereview.appspot.com/6847128/diff/5002/pkg/present/style_test.go#ne...
pkg/present/style_test.go:22: {"hey [[http://go/go][Gophers]] around",
use a real url in these examples, like http://golang.org/doc/ or something.

https://codereview.appspot.com/6847128/diff/5002/pkg/present/style_test.go#ne...
pkg/present/style_test.go:60: {"_hey_ [[http://go/go][*Gophers*]] *around*",
please add a test case for a link text with multiple words

https://codereview.appspot.com/6847128/diff/5002/pkg/present/style_test.go#ne...
pkg/present/style_test.go:61: "<i>hey</i> <a
href=\"http://go/go\"><b>Gophers</b></a> <b>around</b>"},
use `backquotes` to avoid the escaping

https://codereview.appspot.com/6847128/diff/5002/pkg/present/style_test.go#ne...
pkg/present/style_test.go:62: {"Visit [[http://go/go]] now", "Visit <a
href=\"http://go/go\">http://go/go</a> now"},
ditto

[francesc, 2012-12-01 20:53:57]

PTAL, I added more tests too

https://codereview.appspot.com/6847128/diff/5002/pkg/present/link.go
File pkg/present/link.go (right):

https://codereview.appspot.com/6847128/diff/5002/pkg/present/link.go#newcode50
pkg/present/link.go:50: return template.HTML(fmt.Sprintf(`<a href=%q>%s</a>`,
url.String(), label)), nil
On 2012/12/01 11:41:20, adg wrote:
> change this to
>   return template.HTML(renderLink(url.String(), label)), nil
> 
> and add this function:
> 
> func renderLink(url, text string) string {
>     if text == "" {
>         text = url
>     }
>     return fmt.Sprintf(`<a href="%s" target="_blank">%s</a>`, url, text)
> }

Done.

https://codereview.appspot.com/6847128/diff/5002/pkg/present/link.go#newcode53
pkg/present/link.go:53: var inlineExp =
regexp.MustCompile(`^\[\[([^\[\]]+)\](\[([^\[\]]*)\])?\]`)
On 2012/12/01 11:41:20, adg wrote:
> Gross regexp. Please read:
>  
>
http://commandcenter.blogspot.sg/2011/08/regular-expressions-in-lexing-and.html
> 
> I think this would be much nicer as a simple function that does everything the
> obvious way:
> 
> // parseInlineLink parses an inline link at the start of s, and returns
> // a rendered HTML link and the total length of the raw inline link.
> // If no inline link is present, it returns all zeroes.
> func parseInlineLink(s string) (link string, length int) {
> 	if len(s) < 2 || s[0] != '[' || s[1] != '[' {
> 		return
> 	}
> 	end := strings.Index(s, "]]")
> 	if end == -1 {
> 		return
> 	}
> 	urlEnd := strings.Index(s, "]")
> 	if urlEnd == -1 {
> 		return
> 	}
> 	url := s[2:urlEnd]
> 	const badURLChars = `<>"{}|\^~[]` + "`"	// per RFC1738 section 2.2
> 	if strings.ContainsAny(url, badURLChars) {
> 		return
> 	}
> 	if urlEnd == end {
> 		return renderLink(url, ""), end + 2
> 	}
> 	if s[urlEnd:urlEnd+2] != "][" {
> 		return
> 	}
> 	text := s[urlEnd+2 : end]
> 	return renderLink(url, text), end + 2
> }
> 
> now the other functions are unnecessary.

Done, but removed the check after finding urlEnd, since it's logically
impossible

https://codereview.appspot.com/6847128/diff/5002/pkg/present/style.go
File pkg/present/style.go (right):

https://codereview.appspot.com/6847128/diff/5002/pkg/present/style.go#newcode49
pkg/present/style.go:49: if isInlineLink(word) {
On 2012/12/01 11:41:20, adg wrote:
> if link, _ := parseInlineLink(word); link != "" {
>     words[w] = link
>     continue Word
> }

Done.

https://codereview.appspot.com/6847128/diff/5002/pkg/present/style.go#newcode132
pkg/present/style.go:132: if start, end, found := findInlineLink(s, i); found {
On 2012/12/01 11:41:20, adg wrote:
> if _, length := parseInlineLink(s[i:]) length > 0 {
>   words = append(s[i:i+length])
>   mark = i+length
> }

Done.

https://codereview.appspot.com/6847128/diff/5002/pkg/present/style_test.go
File pkg/present/style_test.go (right):

https://codereview.appspot.com/6847128/diff/5002/pkg/present/style_test.go#ne...
pkg/present/style_test.go:22: {"hey [[http://go/go][Gophers]] around",
On 2012/12/01 11:41:20, adg wrote:
> use a real url in these examples, like http://golang.org/doc/ or something.

Done.

https://codereview.appspot.com/6847128/diff/5002/pkg/present/style_test.go#ne...
pkg/present/style_test.go:60: {"_hey_ [[http://go/go][*Gophers*]] *around*",
On 2012/12/01 11:41:20, adg wrote:
> please add a test case for a link text with multiple words

Done.

https://codereview.appspot.com/6847128/diff/5002/pkg/present/style_test.go#ne...
pkg/present/style_test.go:61: "<i>hey</i> <a
href=\"http://go/go\"><b>Gophers</b></a> <b>around</b>"},
On 2012/12/01 11:41:20, adg wrote:
> use `backquotes` to avoid the escaping

Done.

https://codereview.appspot.com/6847128/diff/5002/pkg/present/style_test.go#ne...
pkg/present/style_test.go:62: {"Visit [[http://go/go]] now", "Visit <a
href=\"http://go/go\">http://go/go</a> now"},
On 2012/12/01 11:41:20, adg wrote:
> ditto

Done.

[adg, 2012-12-02 06:06:53]

https://codereview.appspot.com/6847128/diff/19001/pkg/present/link_test.go
File pkg/present/link_test.go (right):

https://codereview.appspot.com/6847128/diff/19001/pkg/present/link_test.go#ne...
pkg/present/link_test.go:20: {"[[http://golang.org]]", "http://golang.org",
"http://golang.org", 21},
add at least one test where the string doesn't begin with "[["

https://codereview.appspot.com/6847128/diff/19001/pkg/present/link_test.go#ne...
pkg/present/link_test.go:28: {"[[http:// *spaces* .com]]", "http:// *spaces*
.com", "http:// *spaces* .com", 25},
I think we should disallow spaces in the URL, too. The RFC calls them out
specifically.

https://codereview.appspot.com/6847128/diff/19001/pkg/present/link_test.go#ne...
pkg/present/link_test.go:36: if a := fmt.Sprintf(tmpl, test.link, test.text);
length != test.length || link != a {
might as well use renderLink here.

[francesc, 2012-12-03 05:13:36]

https://codereview.appspot.com/6847128/diff/19001/pkg/present/link_test.go
File pkg/present/link_test.go (right):

https://codereview.appspot.com/6847128/diff/19001/pkg/present/link_test.go#ne...
pkg/present/link_test.go:20: {"[[http://golang.org]]", "http://golang.org",
"http://golang.org", 21},
On 2012/12/02 06:06:53, adg wrote:
> add at least one test where the string doesn't begin with "[["

Done.

https://codereview.appspot.com/6847128/diff/19001/pkg/present/link_test.go#ne...
pkg/present/link_test.go:28: {"[[http:// *spaces* .com]]", "http:// *spaces*
.com", "http:// *spaces* .com", 25},
On 2012/12/02 06:06:53, adg wrote:
> I think we should disallow spaces in the URL, too. The RFC calls them out
> specifically.

Done.

https://codereview.appspot.com/6847128/diff/19001/pkg/present/link_test.go#ne...
pkg/present/link_test.go:36: if a := fmt.Sprintf(tmpl, test.link, test.text);
length != test.length || link != a {
On 2012/12/02 06:06:53, adg wrote:
> might as well use renderLink here.

Done.

[adg, 2012-12-03 05:20:07]

LGTM modulo these documentation changes.

https://codereview.appspot.com/6847128/diff/20003/pkg/present/doc.go
File pkg/present/doc.go (right):

https://codereview.appspot.com/6847128/diff/20003/pkg/present/doc.go#newcode78
pkg/present/doc.go:78: Links can be inlined in the text using the syntax
[[url][text]], or
Links can be included in any text with the form [[url][label]], or [[url]] to
use the URL itself as the label.

(drop the part about font modifiers. That should be a given.

https://codereview.appspot.com/6847128/diff/20003/pkg/present/doc.go#newcode82
pkg/present/doc.go:82: Note that characters `[` and `]` are not allowed in
neither url nor text.
actually [] can be used inside the text provided it's not ]]. I think you can
remove this line too.

[francesc, 2012-12-03 05:25:38]

https://codereview.appspot.com/6847128/diff/20003/pkg/present/doc.go
File pkg/present/doc.go (right):

https://codereview.appspot.com/6847128/diff/20003/pkg/present/doc.go#newcode78
pkg/present/doc.go:78: Links can be inlined in the text using the syntax
[[url][text]], or
On 2012/12/03 05:20:07, adg wrote:
> Links can be included in any text with the form [[url][label]], or [[url]] to
> use the URL itself as the label.
> 
> (drop the part about font modifiers. That should be a given.

Done.

https://codereview.appspot.com/6847128/diff/20003/pkg/present/doc.go#newcode82
pkg/present/doc.go:82: Note that characters `[` and `]` are not allowed in
neither url nor text.
On 2012/12/03 05:20:07, adg wrote:
> actually [] can be used inside the text provided it's not ]]. I think you can
> remove this line too.

Done.

[adg, 2012-12-03 05:32:27]

LGTM

[francesc, 2012-12-03 05:33:44]

*** Submitted as
https://code.google.com/p/go/source/detail?r=c0f111ea557b&repo=talks ***

go.talks/pkg/present: Adding inline links with style.

Read doc.go for more details.

R=golang-dev, adg
CC=golang-dev
https://codereview.appspot.com/6847128