code review 6907045: go.talks/pkg/present: fix header parsing; make subtitle...

go.talks/pkg/present: fix header parsing; make subtitle optional

Fixes issue 4497.

[adg, 2012-12-06 23:38:12]

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

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

[gri, 2012-12-07 01:32:54]

LGTM

https://codereview.appspot.com/6907045/diff/1/pkg/present/parse.go
File pkg/present/parse.go (right):

https://codereview.appspot.com/6907045/diff/1/pkg/present/parse.go#newcode230
pkg/present/parse.go:230: if mode&TitlesOnly > 0 {
s/>/!=/

is more canonical (w/ > I am tempted to figure out if there's more meaning to
it)

https://codereview.appspot.com/6907045/diff/1/pkg/present/parse.go#newcode373
pkg/present/parse.go:373: if doc.Subtitle == "" {
I would negate the condition and get rid of the continue:

if doc.Subtitle != "" {
   return fmt.Errorf("unexpected header line: %q", text)
}
doc.Subtitle = text

[adg, 2012-12-07 02:47:13]

Thanks

https://codereview.appspot.com/6907045/diff/1/pkg/present/parse.go
File pkg/present/parse.go (right):

https://codereview.appspot.com/6907045/diff/1/pkg/present/parse.go#newcode230
pkg/present/parse.go:230: if mode&TitlesOnly > 0 {
On 2012/12/07 01:32:54, gri wrote:
> s/>/!=/
> 
> is more canonical (w/ > I am tempted to figure out if there's more meaning to
> it)

Done.

https://codereview.appspot.com/6907045/diff/1/pkg/present/parse.go#newcode373
pkg/present/parse.go:373: if doc.Subtitle == "" {
I'd prefer to leave it this way, because this and the preceding two if
statements test for a specific line condition and act accordingly.

[adg, 2012-12-07 02:48:02]

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

go.talks/pkg/present: fix header parsing; make subtitle optional

Fixes issue 4497.

R=gri
CC=golang-dev
https://codereview.appspot.com/6907045