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

[a...@golang.org]

Reviewers: golang-dev1,

Message:
Hello golan...@googlegroups.com,

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


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

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

Please review this at https://codereview.appspot.com/10539043/

Affected files:
M pkg/present/code.go

[dvy...@google.com]

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#newcode173
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#newcode181
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

https://codereview.appspot.com/10539043/

[a...@golang.org]

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#newcode173
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#newcode181
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.

https://codereview.appspot.com/10539043/

[dvy...@google.com]

looks good, but please wait for somebody else

https://codereview.appspot.com/10539043/

[r...@golang.org]

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#newcode150
pkg/present/code.go:150: `))
this really doesn't have to be one declaration. i can't see the forest
for the trees.

https://codereview.appspot.com/10539043/

[a...@golang.org]

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#newcode150
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.

https://codereview.appspot.com/10539043/

[r...@golang.org]

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#newcode139
pkg/present/code.go:139: return ""
leadingSpaceRE = regexp.MustCompile("^[ \t]*")
return leadingSpaceRE.FindString(s)

in any case you don't want unicode.IsSpace

https://codereview.appspot.com/10539043/

[a...@golang.org]

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#newcode139
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.

https://codereview.appspot.com/10539043/