code review 624041: template: fixed html formatter bug where it would turn ... (issue624041)
a...@golang.org
Message:
Hello r, rsc (cc: golan...@googlegroups.com),
I'd like you to review this change.
Description:
template: fixed html formatter bug where it would turn a []byte
into a string of decimal numbers.
Please review this at http://codereview.appspot.com/624041/show
Affected files:
M src/pkg/template/format.go
M src/pkg/template/template_test.go
Index: src/pkg/template/format.go
===================================================================
--- a/src/pkg/template/format.go
+++ b/src/pkg/template/format.go
@@ -62,6 +62,6 @@
// HTMLFormatter formats arbitrary values for HTML
func HTMLFormatter(w io.Writer, value interface{}, format string) {
var b bytes.Buffer
- fmt.Fprint(&b, value)
+ StringFormatter(&b, value, "")
HTMLEscape(w, b.Bytes())
}
Index: src/pkg/template/template_test.go
===================================================================
--- a/src/pkg/template/template_test.go
+++ b/src/pkg/template/template_test.go
@@ -565,3 +565,14 @@
t.Errorf("for %q: expected %q got %q", input, expect, buf.String())
}
}
+
+func TestHTMLFormatterWithByte(t *testing.T) {
+ s := "Test string."
+ b := []byte(s)
+ var buf bytes.Buffer
+ HTMLFormatter(&buf, b, "")
+ bs := buf.String()
+ if bs != s {
+ t.Errorf("munged []byte, expected: %s got: %s", s, bs)
+ }
+}
Andrew Gerrand
StringFormatter() on it first. I can't decide if this is a good or bad
thing.
Russ Cox
> This means any custom formatter expecting a string should call
> StringFormatter() on it first. I can't decide if this is a good or bad
> thing.
Doesn't it mean that any custom formatter expecting a []byte
should call StringFormatter on it first?
Russ
Andrew Gerrand
Kind-of. Shouldn't we allow users of template to throw []byte into a
template and reasonably expect it to be treated as a string? (unless
the formatter used is specifically designed to do something fancy with
[]byte).
I found it surprising that simply adding the html formatter gave me a
dramatically different (and useless) output.
Andrew
Russ Cox
>> should call StringFormatter on it first?
>
> Kind-of. Shouldn't we allow users of template to throw []byte into a
> template and reasonably expect it to be treated as a string? (unless
> the formatter used is specifically designed to do something fancy with
> []byte).
>
> I found it surprising that simply adding the html formatter gave me a
> dramatically different (and useless) output.
I agree that HTMLFormatter has a bug. But you sound like
you think there's a deeper problem.
Russ
Andrew Gerrand
It just seems odd that any new formatter written should have to wrap
the input in StringFormatter to get a string, as I imagine _most_
formatters will expect/want a string.
It's probably not a big deal. I can't think of a better way of
approaching it right now. Maybe it's fine.
Andrew
Rob 'Commander' Pike
-rob
Andrew Gerrand
> i wonder if it would be simpler for Execute to promote a []byte to a string for you so it would work in all formatters. i need to think about this.
My counter-example for this instance is: what if I write a
HexFormatter which takes a []byte and outputs a nice hexdump?
Rob 'Commander' Pike
On Mar 17, 2010, at 11:32 PM, Andrew Gerrand wrote:
> On 18 March 2010 17:23, Rob 'Commander' Pike <r...@google.com> wrote:
>> i wonder if it would be simpler for Execute to promote a []byte to a string for you so it would work in all formatters. i need to think about this.
>
> My counter-example for this instance is: what if I write a
> HexFormatter which takes a []byte and outputs a nice hexdump?
win some, lose some. it may be that custom formatters must be aware. that doesn't seem unreasonable but i want to sleep on it.
-rob
Russ Cox
> the input in StringFormatter to get a string, as I imagine _most_
> formatters will expect/want a string.
You don't actually have to pass it through StringFormatter.
In the case of HTMLFormatter, it would have been more
efficient to do:
b, ok := value.([]byte)
if !ok {
var buf bytes.Buffer
fmt.Fprint(&buf, value)
b = buf.Bytes()
}
Converting []byte to string automatically would require
going back to []byte so that you can call HTMLEscape,
which is two unnecessary conversions. It's probably not
a great solution in general.
The most interesting formatters, at least in the template code
I've written or seen, are the ones that take a particular
rich data type and do something type-specific. For example,
godoc has formatters that format a syntax tree as HTML or
as text. The directory listings have a tiny little formatter
called dir/ that takes a *os.Dir and emits a "/" if it is a
directory, and "" otherwise, which puts the / on adler32/ on
the http://golang.org/src/pkg/hash/ page. There are probably
more that I'm forgetting, but it's important not to make those
harder.
One possibility would be to change the signature of the
formatters to allow
func Formatter(w io.Writer, value T, format string)
for any type T, and have Execute treat T = string or T = []byte
as meaning "if it's not that type already, convert to a string
representation and then pass that type". So you'd have
func HTMLFormatter(w io.Writer, value []byte, format string)
and Execute would then do the conversions, but
func DirSlash(w io.Writer, value *os.Dir, format string)
func HTMLAst(w io.Writer, value ast.Expr, format string)
would still work too (and Execute could give a good error
when the value isn't the expected type).
That opens the door to the possibility of using formatters
that accept []byte in pseudo-pipelines, like
{foo|ast|html}
as long as the ast formatter can handle foo and then the
html formatter can handle []byte. Execute would run "ast"
writing to a bytes.Buffer and then run "html" passing
buf.Bytes() writing to wherever it usually writes to.
The reflect package didn't have Call when we wrote template,
so this wasn't an option we could have even considered.
Json-template does pipelines now but there it's easier to
just keep returning new values and taking old values, because
there are no static types in the program.
Russ
Rob 'Commander' Pike
let's check in this fix but please open an issue and include rsc's
mail. this is the second time a rewrite of the template
formatters has come up and we should address it.
Andrew Gerrand
> let's check in this fix but please open an issue and include rsc's mail.
> this is the second time a rewrite of the template
> formatters has come up and we should address it.
a...@golang.org
Please take another look.
Rob 'Commander' Pike
> To unsubscribe from this group, send email to golang-dev
> +unsubscribegooglegroups.com or reply to this email with the words
> "REMOVE ME" as the subject.
a...@golang.org
http://code.google.com/p/go/source/detail?r=9a43d0a09c00 ***
template: fixed html formatter bug where it would turn a []byte
into a string of decimal numbers.
R=r, rsc
CC=golang-dev
http://codereview.appspot.com/624041