code review 10539043: go.talks/pkg/present: include line numbers in output HTML

go.talks/pkg/present: include line numbers in output HTML

Also refactor HTML code generation to be line and template based.

[adg, 2013-06-25 10:48:16]

Hello golang-dev@googlegroups.com,

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

[dvyukov, 2013-06-25 11:02:16]

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

https://codereview.appspot.com/10539043/diff/19001/pkg/present/code.go#newcod...
pkg/present/code.go:173: startLine++
have you tested it for off-by-one bugs?

https://codereview.appspot.com/10539043/diff/19001/pkg/present/code.go#newcod...
pkg/present/code.go:181: lines = append(lines, codeLine{B: s.Bytes(), N: n})
you need to copy s.Bytes() here
it points into underlying scanner slice that will be overwritten

[adg, 2013-06-25 11:11:22]

PTAL

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

https://codereview.appspot.com/10539043/diff/19001/pkg/present/code.go#newcod...
pkg/present/code.go:173: startLine++
On 2013/06/25 11:02:16, dvyukov wrote:
> have you tested it for off-by-one bugs?

I've tested it. It works. Lines are 1-indexed.

https://codereview.appspot.com/10539043/diff/19001/pkg/present/code.go#newcod...
pkg/present/code.go:181: lines = append(lines, codeLine{B: s.Bytes(), N: n})
On 2013/06/25 11:02:16, dvyukov wrote:
> you need to copy s.Bytes() here
> it points into underlying scanner slice that will be overwritten

oops! For some reason I had in mind that it was pulling it out of the underlying
byte slice, which is impossible of course. Since I have to copy here, I'll just
use strings anyway which is more succinct in other places.

[dvyukov, 2013-06-25 11:21:11]

looks good, but please wait for somebody else

[r, 2013-06-25 16:12:40]

https://codereview.appspot.com/10539043/diff/28001/pkg/present/code.go
File pkg/present/code.go (right):

https://codereview.appspot.com/10539043/diff/28001/pkg/present/code.go#newcod...
pkg/present/code.go:150: `))
this really doesn't have to be one declaration. i can't see the forest for the
trees.

[adg, 2013-06-25 23:05:57]

PTAL

https://codereview.appspot.com/10539043/diff/28001/pkg/present/code.go
File pkg/present/code.go (right):

https://codereview.appspot.com/10539043/diff/28001/pkg/present/code.go#newcod...
pkg/present/code.go:150: `))
On 2013/06/25 16:12:40, r wrote:
> this really doesn't have to be one declaration. i can't see the forest for the
> trees.

Done.

[r, 2013-06-25 23:37:33]

https://codereview.appspot.com/10539043/diff/33001/pkg/present/code.go
File pkg/present/code.go (right):

https://codereview.appspot.com/10539043/diff/33001/pkg/present/code.go#newcod...
pkg/present/code.go:139: return ""
leadingSpaceRE = regexp.MustCompile("^[ \t]*")
return leadingSpaceRE.FindString(s)

in any case you don't want unicode.IsSpace

[adg, 2013-06-25 23:44:17]

PTAL

https://codereview.appspot.com/10539043/diff/33001/pkg/present/code.go
File pkg/present/code.go (right):

https://codereview.appspot.com/10539043/diff/33001/pkg/present/code.go#newcod...
pkg/present/code.go:139: return ""
On 2013/06/25 23:37:33, r wrote:
> leadingSpaceRE = regexp.MustCompile("^[ \t]*")
> return leadingSpaceRE.FindString(s)
> 
> in any case you don't want unicode.IsSpace

Done.

[r, 2013-06-26 00:01:54]

LGTM
much easier to read

[adg, 2013-06-26 00:03:35]

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

go.talks/pkg/present: include line numbers in output HTML

Also refactor HTML code generation to be line and template based.

R=golang-dev, dvyukov, r
CC=golang-dev
https://codereview.appspot.com/10539043