code review 4127043: template: Add simple formatter chaining.

template: Add simple formatter chaining.

Fixes issue 676.

[Kyle C, 2011-01-31 20:49:34]

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

I'd like you to review this change.

[r, 2011-01-31 21:33:20]

http://codereview.appspot.com/4127043/diff/2001/src/pkg/template/template.go
File src/pkg/template/template.go (right):

http://codereview.appspot.com/4127043/diff/2001/src/pkg/template/template.go#...
src/pkg/template/template.go:65: the string representations of the fields.
want to mention pipelining

http://codereview.appspot.com/4127043/diff/2001/src/pkg/template/template.go#...
src/pkg/template/template.go:407: func (t *Template) getFormatter(name string)
func(io.Writer, string, ...interface{}) {
needs a comment

http://codereview.appspot.com/4127043/diff/2001/src/pkg/template/template.go#...
src/pkg/template/template.go:431: formatters = strings.Split(lastWord[bar+1:],
"|", -1)
i've been meaning to do this for a while. trim the spaces from each entry

http://codereview.appspot.com/4127043/diff/2001/src/pkg/template/template.go#...
src/pkg/template/template.go:441: t.parseError("unknown formatter: %s", f)
s/%s/%q/

http://codereview.appspot.com/4127043/diff/2001/src/pkg/template/template.go#...
src/pkg/template/template.go:725: var buf bytes.Buffer
this is too expensive in the common case. don't allocate a buffer and do the
copying if there's only one formatter.

[rsc, 2011-01-31 23:39:31]

there should be a "fixes issue" in the CL description

http://codereview.appspot.com/4127043/diff/2001/src/pkg/template/template.go
File src/pkg/template/template.go (right):

http://codereview.appspot.com/4127043/diff/2001/src/pkg/template/template.go#...
src/pkg/template/template.go:71: "bytes"
sort import list

http://codereview.appspot.com/4127043/diff/2001/src/pkg/template/template.go#...
src/pkg/template/template.go:407: func (t *Template) getFormatter(name string)
func(io.Writer, string, ...interface{}) {
get is almost never needed in a name in Go.

s/getFormatter/formatter/

[Kyle C, 2011-02-01 00:42:15]

http://codereview.appspot.com/4127043/diff/2001/src/pkg/template/template.go
File src/pkg/template/template.go (right):

http://codereview.appspot.com/4127043/diff/2001/src/pkg/template/template.go#...
src/pkg/template/template.go:65: the string representations of the fields.
On 2011/01/31 21:33:20, r wrote:
> want to mention pipelining

Done, but wording could certainly be improved.

http://codereview.appspot.com/4127043/diff/2001/src/pkg/template/template.go#...
src/pkg/template/template.go:71: "bytes"
On 2011/01/31 23:39:31, rsc wrote:
> sort import list

Done.

http://codereview.appspot.com/4127043/diff/2001/src/pkg/template/template.go#...
src/pkg/template/template.go:407: func (t *Template) getFormatter(name string)
func(io.Writer, string, ...interface{}) {
On 2011/01/31 21:33:20, r wrote:
> needs a comment

Done.

http://codereview.appspot.com/4127043/diff/2001/src/pkg/template/template.go#...
src/pkg/template/template.go:407: func (t *Template) getFormatter(name string)
func(io.Writer, string, ...interface{}) {
On 2011/01/31 23:39:31, rsc wrote:
> get is almost never needed in a name in Go.
> 
> s/getFormatter/formatter/
> 

Done.

http://codereview.appspot.com/4127043/diff/2001/src/pkg/template/template.go#...
src/pkg/template/template.go:431: formatters = strings.Split(lastWord[bar+1:],
"|", -1)
On 2011/01/31 21:33:20, r wrote:
> i've been meaning to do this for a while. trim the spaces from each entry

Well, locally there is little purpose in trimming the spaces for each entry,
because words() has already split on whitespace. Unless I'm mis-reading
something, to allow "{foo | f1 | f2 }" we'll either need to rework "analyze()"
to tokenize more intelligently, or pass in the entire contents of the variable
expression into newVariable().
Either way, happy to do it, but it sounds like it might be better done in
another change.

http://codereview.appspot.com/4127043/diff/2001/src/pkg/template/template.go#...
src/pkg/template/template.go:441: t.parseError("unknown formatter: %s", f)
On 2011/01/31 21:33:20, r wrote:
> s/%s/%q/

Done.

http://codereview.appspot.com/4127043/diff/2001/src/pkg/template/template.go#...
src/pkg/template/template.go:725: var buf bytes.Buffer
On 2011/01/31 21:33:20, r wrote:
> this is too expensive in the common case. don't allocate a buffer and do the
> copying if there's only one formatter.

Yeah, initially I special-cased it for single formatter, but it resulted in some
duplication, so I decided to see if reviewers cared.
Added back.

[Kyle C, 2011-02-01 00:42:39]

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

Please take another look.

[r, 2011-02-01 01:24:32]

http://codereview.appspot.com/4127043/diff/6002/src/pkg/template/template.go
File src/pkg/template/template.go (right):

http://codereview.appspot.com/4127043/diff/6002/src/pkg/template/template.go#...
src/pkg/template/template.go:68: with the each formatter operating on the bytes
generated by the previous one.
Multiple formatters separated by the pipeline character | are executed
sequentially, with each formatter receiving the bytes emitted by the one
to its left.

http://codereview.appspot.com/4127043/diff/6002/src/pkg/template/template.go#...
src/pkg/template/template.go:411: // Returns the formatter with the given name,
and nil if it isn't found.
formatter returns the Formatter with the given name in the Template, or nil if
none exists.

http://codereview.appspot.com/4127043/diff/6002/src/pkg/template/template.go#...
src/pkg/template/template.go:414: if fn, ok := t.fmap[name]; ok {
if t.fmap != nil {
   if fn := t.fmap[name]; fn != nil {
      return fn
   }
}
return builtins[name]

http://codereview.appspot.com/4127043/diff/6002/src/pkg/template/template.go#...
src/pkg/template/template.go:430: // the form "|fmt".  For example: {a b c|d}
this comment is wrong now

http://codereview.appspot.com/4127043/diff/6002/src/pkg/template/template.go#...
src/pkg/template/template.go:431: formatters := []string{""}
i don't see why formatters starts out like this. why isn't it just a slice of
names?

http://codereview.appspot.com/4127043/diff/6002/src/pkg/template/template.go#...
src/pkg/template/template.go:740: var buf bytes.Buffer
maybe you should declare a pair of buffers and ping-pong. this is clumsy. i
think if you do that, you don't need to special case len==1 because you always
write to st.wr on the last element.

http://codereview.appspot.com/4127043/diff/6002/src/pkg/template/template_tes...
File src/pkg/template/template_test.go (right):

http://codereview.appspot.com/4127043/diff/6002/src/pkg/template/template_tes...
src/pkg/template/template_test.go:682: var cases = []struct {
move these tests outside the function, as they are in the other examples, and
add some that demonstrate error reporting

[Kyle C, 2011-02-03 17:21:28]

PTAL.

http://codereview.appspot.com/4127043/diff/6002/src/pkg/template/template.go
File src/pkg/template/template.go (right):

http://codereview.appspot.com/4127043/diff/6002/src/pkg/template/template.go#...
src/pkg/template/template.go:68: with the each formatter operating on the bytes
generated by the previous one.
On 2011/02/01 01:24:33, r wrote:
> Multiple formatters separated by the pipeline character | are executed
> sequentially, with each formatter receiving the bytes emitted by the one
> to its left.

Much better. Done.

http://codereview.appspot.com/4127043/diff/6002/src/pkg/template/template.go#...
src/pkg/template/template.go:411: // Returns the formatter with the given name,
and nil if it isn't found.
On 2011/02/01 01:24:33, r wrote:
> formatter returns the Formatter with the given name in the Template, or nil if
> none exists.

Done.

http://codereview.appspot.com/4127043/diff/6002/src/pkg/template/template.go#...
src/pkg/template/template.go:414: if fn, ok := t.fmap[name]; ok {
On 2011/02/01 01:24:33, r wrote:
> if t.fmap != nil {
>    if fn := t.fmap[name]; fn != nil {
>       return fn
>    }
> }
> return builtins[name]

Done.

http://codereview.appspot.com/4127043/diff/6002/src/pkg/template/template.go#...
src/pkg/template/template.go:430: // the form "|fmt".  For example: {a b c|d}
On 2011/02/01 01:24:33, r wrote:
> this comment is wrong now

Done.

http://codereview.appspot.com/4127043/diff/6002/src/pkg/template/template.go#...
src/pkg/template/template.go:431: formatters := []string{""}
On 2011/02/01 01:24:33, r wrote:
> i don't see why formatters starts out like this. why isn't it just a slice of
> names?

When no formatters are explicitly given, there is one formatter: the default
formatter, with the name of "". So, until we find a pipe and parse out
formatters, "formatters" is a slice with one empty string.

I've added a comment to clarify this.

http://codereview.appspot.com/4127043/diff/6002/src/pkg/template/template.go#...
src/pkg/template/template.go:740: var buf bytes.Buffer
On 2011/02/01 01:24:33, r wrote:
> maybe you should declare a pair of buffers and ping-pong. this is clumsy. i
> think if you do that, you don't need to special case len==1 because you always
> write to st.wr on the last element.

Ok, I've modified it to avoid the len(v.formatters) == 1 special case and to use
a pair of buffers to alternate between while avoiding allocating them
unnecessarily. It's a bit intricate, so I'm not sure how much better it is.

http://codereview.appspot.com/4127043/diff/6002/src/pkg/template/template_tes...
File src/pkg/template/template_test.go (right):

http://codereview.appspot.com/4127043/diff/6002/src/pkg/template/template_tes...
src/pkg/template/template_test.go:682: var cases = []struct {
On 2011/02/01 01:24:33, r wrote:
> move these tests outside the function, as they are in the other examples, and
> add some that demonstrate error reporting

Done, somewhat; I'm interested in suggestions for additional test cases if you
have them. Should I perhaps just be adding multi-formatter tests to the existing
huge list of test cases instead of making my own TestChained?

[rsc, 2011-02-03 18:07:47]

http://codereview.appspot.com/4127043/diff/12001/src/pkg/template/template.go
File src/pkg/template/template.go (right):

http://codereview.appspot.com/4127043/diff/12001/src/pkg/template/template.go...
src/pkg/template/template.go:728: var destbuf, prevbuf *bytes.Buffer
These should be in the state.
Also the loop seems to complex to me.
If you declare buf [2]bytes.Buffer in the state
and a helper method t.format(w, fmt, val)
that does the fn lookup and execerror and
calls fn(w, fmt, val...) then you can write:

for i, fmt := range v.fmt[:len(v.fmt)-1] {
    b := &st.buf[i&1]
    b.Reset()
    t.format(b, fmt, val)
    val = val[0:1]
    val[0] = b.Bytes()
}
t.format(st.wr, v.fmt[len(v.fmt)-1], val)

[Kyle C, 2011-02-03 20:39:51]

PTAL

http://codereview.appspot.com/4127043/diff/12001/src/pkg/template/template.go
File src/pkg/template/template.go (right):

http://codereview.appspot.com/4127043/diff/12001/src/pkg/template/template.go...
src/pkg/template/template.go:728: var destbuf, prevbuf *bytes.Buffer
Yeah. That's much better, thanks. Done.



On 2011/02/03 18:07:47, rsc wrote:
> These should be in the state.
> Also the loop seems to complex to me.
> If you declare buf [2]bytes.Buffer in the state
> and a helper method t.format(w, fmt, val)
> that does the fn lookup and execerror and
> calls fn(w, fmt, val...) then you can write:
> 
> for i, fmt := range v.fmt[:len(v.fmt)-1] {
>     b := &st.buf[i&1]
>     b.Reset()
>     t.format(b, fmt, val)
>     val = val[0:1]
>     val[0] = b.Bytes()
> }
> t.format(st.wr, v.fmt[len(v.fmt)-1], val)
>

[r, 2011-02-03 20:47:38]

looking good

it would be fine to put the formatter tests into the main test array.
or, you could create a second such array for all the formatter tests.

either way, but it is nice to break up the tests a bit.

http://codereview.appspot.com/4127043/diff/2003/src/pkg/template/template.go
File src/pkg/template/template.go (right):

http://codereview.appspot.com/4127043/diff/2003/src/pkg/template/template.go#...
src/pkg/template/template.go:426: // the form "field|fmt", where fmt is one or
more | separated formatter names.
where fmt is one or more formatter names separated by a | character.

[Kyle C, 2011-02-04 23:46:39]

I moved the formatter Tests in with the chained formatter tests and renamed the
test function to TestFormatters. I could've moved mine in with the rest of the
tests, but it seemed nicer to break it up and to not rely on the big complicated
S struct unnecessarily.

http://codereview.appspot.com/4127043/diff/2003/src/pkg/template/template.go
File src/pkg/template/template.go (right):

http://codereview.appspot.com/4127043/diff/2003/src/pkg/template/template.go#...
src/pkg/template/template.go:426: // the form "field|fmt", where fmt is one or
more | separated formatter names.
On 2011/02/03 20:47:39, r wrote:
> where fmt is one or more formatter names separated by a | character.

Done.

[r2, 2011-02-04 23:57:33]

i just checked in a change to template.  please sync, update, and upload.

-rob

[Kyle C, 2011-02-05 00:08:01]

Saw your change, synced, merged, and uploaded before sending my mail. I've
hg upload once more to be sure.


On Fri, Feb 4, 2011 at 3:57 PM, Rob 'Commander' Pike <r@google.com> wrote:

> i just checked in a change to template.  please sync, update, and upload.
>
> -rob
>
>

[r, 2011-02-05 00:16:04]

a couple of comment tweaks but it's looking good

http://codereview.appspot.com/4127043/diff/4002/src/pkg/template/template.go
File src/pkg/template/template.go (right):

http://codereview.appspot.com/4127043/diff/4002/src/pkg/template/template.go#...
src/pkg/template/template.go:75: emitted by the one to its left.
line breaks aren't right.  it might be worth just making this a new paragraph -
add a blank line after 72. it's important enough.

http://codereview.appspot.com/4127043/diff/4002/src/pkg/template/template.go#...
src/pkg/template/template.go:187: buf    [2]bytes.Buffer // buffers to alternate
between for chained formatters.
// alternating buffers used when chaining formatters

http://codereview.appspot.com/4127043/diff/4002/src/pkg/template/template.go#...
src/pkg/template/template.go:433: // the form "field|fmt", where fmt is one or
more formatter names separated
this reads funny now. maybe just

After the final space-separated argument, formatters may be specified separated
by pipe symbols, for example: {a b c|d|e}

[Kyle C, 2011-02-05 00:34:00]

http://codereview.appspot.com/4127043/diff/4002/src/pkg/template/template.go
File src/pkg/template/template.go (right):

http://codereview.appspot.com/4127043/diff/4002/src/pkg/template/template.go#...
src/pkg/template/template.go:75: emitted by the one to its left.
On 2011/02/05 00:16:05, r wrote:
> line breaks aren't right.  it might be worth just making this a new paragraph
-
> add a blank line after 72. it's important enough.

Done.

http://codereview.appspot.com/4127043/diff/4002/src/pkg/template/template.go#...
src/pkg/template/template.go:187: buf    [2]bytes.Buffer // buffers to alternate
between for chained formatters.
On 2011/02/05 00:16:05, r wrote:
> // alternating buffers used when chaining formatters

Done.

http://codereview.appspot.com/4127043/diff/4002/src/pkg/template/template.go#...
src/pkg/template/template.go:433: // the form "field|fmt", where fmt is one or
more formatter names separated
On 2011/02/05 00:16:05, r wrote:
> this reads funny now. maybe just
> 
> After the final space-separated argument, formatters may be specified
separated
> by pipe symbols, for example: {a b c|d|e}

Done.

[r, 2011-02-05 00:36:31]

LGTM

[r, 2011-02-05 00:37:34]

*** Submitted as http://code.google.com/p/go/source/detail?r=204118e30fbd ***

template: Add simple formatter chaining.

Fixes issue 676.

R=r, rsc, r2
CC=golang-dev
http://codereview.appspot.com/4127043

Committer: Rob Pike <r@golang.org>