code review 5434077: html/template: update to new template API

html/template: update to new template API

Not quite done yet but enough is here to review.

Embedding is eliminated so clients can't accidentally reach
methods of text/template.Template that would break the
invariants.

TODO: two test cases still fail in TestErrors; help requested.

TODO later: Add and Clone are unimplemented.
TODO later: address issue 2349

[r, 2011-11-28 05:07:57]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[r, 2011-11-28 18:41:52]

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

Please take another look.

[r2, 2011-11-28 18:43:47]

This is ready for proper review now.

The TODOs can be done in another CL. There's plenty here already.

-rob

[rsc, 2011-11-30 22:34:08]

LGTM

The two commented out test failures are because it says
{{define "z"}}...{{end}}.  If you remove the framing around
the ... then the tests pass.


		{
			`<div{{template "y"}}>` +
				// Illegal starting in stateTag but not in stateText.
				`{{define "y"}} foo<b{{end}}`,
			`"<" in attribute name: " foo<b"`,
		},
		{
			`<script>reverseList = [{{template "t"}}]</script>` +
				// Missing " after recursive call.
				`{{define "t"}}{{if .Tail}}{{template "t" .Tail}}{{end}}{{.Head}}",{{end}}`,
			`: cannot compute output context for template
t$htmltemplate_stateJS_elementScript`,
		},

[rsc, 2011-11-30 22:41:30]

I have already clpatched and fixed the tests (to figure out
what was wrong), so I will submit this.

[rsc, 2011-11-30 22:42:22]

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

html/template: update to new template API

Not quite done yet but enough is here to review.

Embedding is eliminated so clients can't accidentally reach
methods of text/template.Template that would break the
invariants.

TODO later: Add and Clone are unimplemented.
TODO later: address issue 2349

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

Committer: Russ Cox <rsc@golang.org>

[r2, 2011-11-30 22:50:37]

On Nov 30, 2011, at 2:34 PM, Russ Cox wrote:

> LGTM
> 
> The two commented out test failures are because it says
> {{define "z"}}...{{end}}.  If you remove the framing around
> the ... then the tests pass.
> 
> 
> 		{
> 			`<div{{template "y"}}>` +
> 				// Illegal starting in stateTag but not in stateText.
> 				`{{define "y"}} foo<b{{end}}`,
> 			`"<" in attribute name: " foo<b"`,
> 		},
> 		{
> 			`<script>reverseList = [{{template "t"}}]</script>` +
> 				// Missing " after recursive call.
> 				`{{define "t"}}{{if .Tail}}{{template "t"
.Tail}}{{end}}{{.Head}}",{{end}}`,
> 			`: cannot compute output context for template
> t$htmltemplate_stateJS_elementScript`,
> 		},

peculiar. i of course knew about the redefinition, but when i changed the name
the test didn't pass. i've done a number of minor changes so i must have
inadvertently fixed them.

i'd also like mike samuel to vote, if he has time

-rob

[MikeSamuel, 2011-12-05 22:30:16]

http://codereview.appspot.com/5434077/diff/4006/src/pkg/html/template/clone_t...
File src/pkg/html/template/clone_test.go (right):

http://codereview.appspot.com/5434077/diff/4006/src/pkg/html/template/clone_t...
src/pkg/html/template/clone_test.go:56: d.text.Root = cloneList(s.text.Root)
Ok.  d's root still depends on s.text.Root so below we are testing independence
post clone.

http://codereview.appspot.com/5434077/diff/4006/src/pkg/html/template/clone_t...
src/pkg/html/template/clone_test.go:87: s.text.Execute(&b, data)
Ok.  This is an explicit reference to the contained type.

http://codereview.appspot.com/5434077/diff/4006/src/pkg/html/template/escape_...
File src/pkg/html/template/escape_test.go (right):

http://codereview.appspot.com/5434077/diff/4006/src/pkg/html/template/escape_...
src/pkg/html/template/escape_test.go:933: /* TODO THESE TWO FAIL
Will patch in and investigate.

http://codereview.appspot.com/5434077/diff/4006/src/pkg/html/template/templat...
File src/pkg/html/template/template.go (right):

http://codereview.appspot.com/5434077/diff/4006/src/pkg/html/template/templat...
src/pkg/html/template/template.go:27: // ExecuteTemplate applies the template
associated with t that has the given name
81 columns

http://codereview.appspot.com/5434077/diff/4006/src/pkg/html/template/templat...
src/pkg/html/template/template.go:32: return fmt.Errorf("template: no template
%q associated with template %q", name, t.Name())
If lines 31-34 could be replaced

     if tmpl != nil && !tmpl.escaped {

and line 39 were

     return t.text.ExecuteTemplate(...)

then you could delegate the error reporting on a nonexistent template to
tmpl.text.
A panic when
    (t.Lookup(name) == nil) != (t.text.Lookup(name) == nil)
would prevent any possibility that we execute an unescaped template due to key
skew.

http://codereview.appspot.com/5434077/diff/4006/src/pkg/html/template/templat...
src/pkg/html/template/template.go:86: return fmt.Errorf("html/template: Add
unimplemented")
Add -> Clone

http://codereview.appspot.com/5434077/diff/4006/src/pkg/html/template/templat...
src/pkg/html/template/template.go:198: }
Is the body from the text version copied in here just so that we can reset the
escaped bit on reparsed templates, and so that we can create a wrapper for the
first allocated template?

[MikeSamuel, 2011-12-05 23:36:12]

http://codereview.appspot.com/5434077/diff/4006/src/pkg/html/template/escape_...
File src/pkg/html/template/escape_test.go (right):

http://codereview.appspot.com/5434077/diff/4006/src/pkg/html/template/escape_...
src/pkg/html/template/escape_test.go:933: /* TODO THESE TWO FAIL
On 2011/12/05 22:30:17, MikeSamuel wrote:
> Will patch in and investigate.

Looks like rsc already fixed.

[r, 2011-12-06 19:05:56]

http://codereview.appspot.com/5434077/diff/4006/src/pkg/html/template/templat...
File src/pkg/html/template/template.go (right):

http://codereview.appspot.com/5434077/diff/4006/src/pkg/html/template/templat...
src/pkg/html/template/template.go:198: }
yes. if you have a better approach, please let me know.