|
|
Created:
14 years, 1 month ago by Kyle C Modified:
14 years, 1 month ago Reviewers:
CC:
r, rsc, r2, golang-dev Visibility:
Public. |
Descriptiontemplate: Add simple formatter chaining.
Fixes issue 676.
Patch Set 1 #Patch Set 2 : code review 4127043: template: Add simple formatter chaining. #
Total comments: 14
Patch Set 3 : code review 4127043: template: Add simple formatter chaining. #
Total comments: 14
Patch Set 4 : diff -r 48ec378bff75 https://go.googlecode.com/hg/ #
Total comments: 2
Patch Set 5 : diff -r a861348c835b https://go.googlecode.com/hg/ #
Total comments: 2
Patch Set 6 : diff -r a861348c835b https://go.googlecode.com/hg/ #Patch Set 7 : diff -r 475e73445ae1 https://go.googlecode.com/hg/ #Patch Set 8 : diff -r aa76041bc64d https://go.googlecode.com/hg/ #Patch Set 9 : diff -r 0add71baf83a https://go.googlecode.com/hg/ #
Total comments: 6
Patch Set 10 : diff -r 0add71baf83a https://go.googlecode.com/hg/ #MessagesTotal messages: 17
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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/
Sign in to reply to this message.
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.
Sign in to reply to this message.
Hello r, rsc (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
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
Sign in to reply to this message.
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?
Sign in to reply to this message.
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)
Sign in to reply to this message.
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) >
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
i just checked in a change to template. please sync, update, and upload. -rob
Sign in to reply to this message.
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 > >
Sign in to reply to this message.
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}
Sign in to reply to this message.
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.
Sign in to reply to this message.
*** 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>
Sign in to reply to this message.
|