code review 7065071: go.talks/pkg/present: add Tags feature

go.talks/pkg/present: add Tags feature

By tagging articles and slide decks, we can point readers toward
related content.

[adg, 2013-01-11 00:16:42]

Hello golang-dev@googlegroups.com,

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

[campoy, 2013-01-24 01:26:37]

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

https://codereview.appspot.com/7065071/diff/1/pkg/present/doc.go#newcode22
pkg/present/doc.go:22: The subtitle, date, and tags lines are optional.
Explain what tags are used for.

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

https://codereview.appspot.com/7065071/diff/1/pkg/present/parse.go#newcode379
pkg/present/parse.go:379: tags[i] = strings.TrimSpace(tags[i])
Why not:

for _, tag := range tags {
  doc.Tags = append(doc.Tags, strings.TrimSpace(tag)
}

https://codereview.appspot.com/7065071/diff/1/pkg/present/parse.go#newcode383
pkg/present/parse.go:383: doc.Time = t
We are changing the behavior dropping the "break"
Is this intended?

[adg, 2013-01-24 01:40:34]

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

https://codereview.appspot.com/7065071/diff/1/pkg/present/doc.go#newcode22
pkg/present/doc.go:22: The subtitle, date, and tags lines are optional.
On 2013/01/24 01:26:37, campoy wrote:
> Explain what tags are used for.

They can be used for anything. As far as the present package is concerned,
they're just values that end up in the Doc struct's Tags field (which isn't
discussed here).

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

https://codereview.appspot.com/7065071/diff/1/pkg/present/parse.go#newcode379
pkg/present/parse.go:379: tags[i] = strings.TrimSpace(tags[i])
On 2013/01/24 01:26:37, campoy wrote:
> Why not:
> 
> for _, tag := range tags {
>   doc.Tags = append(doc.Tags, strings.TrimSpace(tag)
> }

This mutates the tags in place and then will do a single allocation (if
necessary) to add the tags to doc.Tags.

https://codereview.appspot.com/7065071/diff/1/pkg/present/parse.go#newcode383
pkg/present/parse.go:383: doc.Time = t
On 2013/01/24 01:26:37, campoy wrote:
> We are changing the behavior dropping the "break"
> Is this intended?

Yes, the behavior is changed because we now allow tags to appear after a time
line.

[campoy, 2013-01-24 01:46:14]

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

https://codereview.appspot.com/7065071/diff/1/pkg/present/doc.go#newcode22
pkg/present/doc.go:22: The subtitle, date, and tags lines are optional.
Well, just explain something like that:

The Tags line contains a comma separated list of tags qualifying some aspect of
the current document.

On 2013/01/24 01:40:34, adg wrote:
> On 2013/01/24 01:26:37, campoy wrote:
> > Explain what tags are used for.
> 
> They can be used for anything. As far as the present package is concerned,
> they're just values that end up in the Doc struct's Tags field (which isn't
> discussed here).

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

https://codereview.appspot.com/7065071/diff/1/pkg/present/parse.go#newcode379
pkg/present/parse.go:379: tags[i] = strings.TrimSpace(tags[i])
I think the code I propose is slightly more readable, but I'll accept your code
too.

On 2013/01/24 01:40:34, adg wrote:
> On 2013/01/24 01:26:37, campoy wrote:
> > Why not:
> > 
> > for _, tag := range tags {
> >   doc.Tags = append(doc.Tags, strings.TrimSpace(tag)
> > }
> 
> This mutates the tags in place and then will do a single allocation (if
> necessary) to add the tags to doc.Tags.

https://codereview.appspot.com/7065071/diff/1/pkg/present/parse.go#newcode383
pkg/present/parse.go:383: doc.Time = t
Ok
On 2013/01/24 01:40:34, adg wrote:
> On 2013/01/24 01:26:37, campoy wrote:
> > We are changing the behavior dropping the "break"
> > Is this intended?
> 
> Yes, the behavior is changed because we now allow tags to appear after a time
> line.
>

[adg, 2013-01-24 02:16:24]

On 24 January 2013 12:46, <campoy@google.com> wrote:

> The Tags line contains a comma separated list of tags qualifying some
> aspect of the current document.
>

Done.

[campoy, 2013-01-24 02:26:19]

LGTM

On 2013/01/24 02:16:24, adg wrote:
> On 24 January 2013 12:46, <mailto:campoy@google.com> wrote:
> 
> > The Tags line contains a comma separated list of tags qualifying some
> > aspect of the current document.
> >
> 
> Done.

[adg, 2013-01-24 02:31:59]

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

go.talks/pkg/present: add Tags feature

By tagging articles and slide decks, we can point readers toward
related content.

R=campoy
CC=golang-dev
https://codereview.appspot.com/7065071