code review 5436080: text/template: address a couple of issues for html/template

text/template: address a couple of issues for html/template
- allow Lookup to work on uninitialized templates
- fix bug in add: can't error after parser is stopped
- add Add method for html/template

[r, 2011-11-28 05:05:44]

Hello golang-dev@googlegroups.com,

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

[adg, 2011-11-28 05:10:48]

http://codereview.appspot.com/5436080/diff/1/src/pkg/text/template/template.go
File src/pkg/text/template/template.go (right):

http://codereview.appspot.com/5436080/diff/1/src/pkg/text/template/template.g...
src/pkg/text/template/template.go:106: // Add associates the argument template
with t, and vice versa, so
Does this mean that the argument template is disassociated from the templates
with which it was formerly associated?

[adg, 2011-11-28 05:12:37]

http://codereview.appspot.com/5436080/diff/1/src/pkg/text/template/template.go
File src/pkg/text/template/template.go (right):

http://codereview.appspot.com/5436080/diff/1/src/pkg/text/template/template.g...
src/pkg/text/template/template.go:114: return fmt.Errorf("template: redefinition
of template %q", arg.name)
Shouldn't you be able to clobber an existing template?

With a name like Add I feel like people are going to try and use this, despite
the warning.

[r, 2011-11-28 07:11:06]

http://codereview.appspot.com/5436080/diff/1/src/pkg/text/template/template.go
File src/pkg/text/template/template.go (right):

http://codereview.appspot.com/5436080/diff/1/src/pkg/text/template/template.g...
src/pkg/text/template/template.go:106: // Add associates the argument template
with t, and vice versa, so
On 2011/11/28 05:10:48, adg wrote:
> Does this mean that the argument template is disassociated from the templates
> with which it was formerly associated?

yes

http://codereview.appspot.com/5436080/diff/1/src/pkg/text/template/template.g...
src/pkg/text/template/template.go:114: return fmt.Errorf("template: redefinition
of template %q", arg.name)
it's just a piece of API html/template needs. it has no reason to be more.

[r, 2011-11-28 07:17:41]

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

Please take another look.

[rog, 2011-11-28 14:00:39]

http://codereview.appspot.com/5436080/diff/5/src/pkg/text/template/template.go
File src/pkg/text/template/template.go (right):

http://codereview.appspot.com/5436080/diff/5/src/pkg/text/template/template.g...
src/pkg/text/template/template.go:112: // for other uses.
It seems to me that it should be OK for other people to layer onto template
similarly to html/template, and that this functionality will be required by
anyone that does so. From the comments here, and above, it seems like that's
frowned upon. Why so?

I was wondering if this functionality could be phrased as
AddParseTree(*parse.Tree) (*Template, error) which might more directly represent
what html/template needs to do. I'm probably missing lots of subtleties though.

[r2, 2011-11-28 17:58:35]

right now i'm just trying to get the html/template package working again.
details at the edges of the API have lower priority

-rob

[rsc, 2011-11-28 18:20:08]

LGTM

This is fine to get html/template working but I agree with
Roger that Add(*parse.Tree) might be a better long term API.
It looks like fitting it into html/template would not be too
hard, and it avoids questions like what happens if the two
different *Template being added together define conflicting
func maps.

[r, 2011-11-28 18:43:01]

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

text/template: address a couple of issues for html/template
- allow Lookup to work on uninitialized templates
- fix bug in add: can't error after parser is stopped
- add Add method for html/template

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