code review 624041: template: fixed html formatter bug where it would turn ...

template: fixed html formatter bug where it would turn a []byte
into a string of decimal numbers.

[adg, 2010-03-18 05:22:56]

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

I'd like you to review this change.

[adg, 2010-03-18 05:24:52]

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.

On 18 March 2010 16:22,  <adg@golang.org> wrote:
> Reviewers: r, rsc,
>
> Message:
> Hello r, rsc (cc: golang-dev@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)
> +       }
> +}
>
>
>

[rsc, 2010-03-18 05:45:53]

On Wed, Mar 17, 2010 at 22:24, Andrew Gerrand <adg@golang.org> wrote:
> 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

[adg, 2010-03-18 05:57:32]

On 18 March 2010 16:45, Russ Cox <rsc@golang.org> wrote:
> On Wed, Mar 17, 2010 at 22:24, Andrew Gerrand <adg@golang.org> wrote:
>> 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?

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

[rsc, 2010-03-18 06:07:54]

>> Doesn't it mean that any custom formatter expecting a []byte
>> 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

[adg, 2010-03-18 06:14:10]

On 18 March 2010 17:07, Russ Cox <rsc@golang.org> wrote:
>>> Doesn't it mean that any custom formatter expecting a []byte
>>> 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.

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

[r2, 2010-03-18 06:23:59]

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.

-rob

On Mar 17, 2010, at 10:22 PM, adg@golang.org wrote:

> Reviewers: r, rsc,
> 
> Message:
> Hello r, rsc (cc: golang-dev@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)
> +	}
> +}
> 
> 

[adg, 2010-03-18 06:33:14]

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?

> On Mar 17, 2010, at 10:22 PM, adg@golang.org wrote:
>
>> Reviewers: r, rsc,
>>
>> Message:
>> Hello r, rsc (cc: golang-dev@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)
>> +     }
>> +}
>>
>>
>
>

[r2, 2010-03-18 06:34:20]

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

[rsc, 2010-03-18 06:35:38]

> 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.

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

[r2, 2010-03-18 18:04:33]

LGTM

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.

On Mar 17, 2010, at 11:35 PM, Russ Cox wrote:

>> 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.
>
> 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

[adg, 2010-03-18 22:38:20]

On 19 March 2010 05:04, Rob 'Commander' Pike <r@google.com> wrote:
> 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.

Done: http://code.google.com/p/go/issues/detail?id=676

[adg, 2010-03-18 22:41:04]

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

Please take another look.

[r2, 2010-03-18 22:43:48]

LGTM

On Mar 18, 2010, at 3:41 PM, adg@golang.org wrote:

> Hello r, rsc (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/624041/show
>
> 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.

[adg, 2010-03-18 22:46:46]

*** Submitted as 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