code review 5354045: text/template: make redefinition of a template in a set...

text/template: make redefinition of a template in a set more consistent.
Also make it agree with the documentation. You get an error, unless you're
calling Add explicitly, in which case it panics since that's almost certainly
a bug. The discrepancy was caused by a panic that wasn't turned into
an error along one path; deleted the offending function for clarity.

[r, 2011-11-07 23:56:48]

Hello golang-dev@googlegroups.com,

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

[Gus Soden, 2011-11-08 19:58:05]

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

http://codereview.appspot.com/5354045/diff/1/src/pkg/text/template/set.go#new...
src/pkg/text/template/set.go:115: err = s.add(tmpl)
This does not fix the problem. The Set's add method returns an error for
duplicate template names.  Perhaps this should be:

   s.tmpl[t.name] = t
   t.set = s

The erorr checking in Set's add method is not needed.

[Gus Soden, 2011-11-08 20:09:44]

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

http://codereview.appspot.com/5354045/diff/1/src/pkg/text/template/set.go#new...
src/pkg/text/template/set.go:115: err = s.add(tmpl)
On 2011/11/08 19:58:05, augustsoden wrote:
> This does not fix the problem. The Set's add method returns an error for
> duplicate template names.  Perhaps this should be:
> 
>    s.tmpl[t.name] = t
>    t.set = s
> 
> The erorr checking in Set's add method is not needed.

Oops, that should be:

        s.tmpl[tmpl.name] = tmpl
        tmpl.set = s

[r2, 2011-11-08 21:21:19]

On Nov 8, 2011, at 11:58 AM, augustsoden@rocketmail.com wrote:

> 
> http://codereview.appspot.com/5354045/diff/1/src/pkg/text/template/set.go
> File src/pkg/text/template/set.go (right):
> 
>
http://codereview.appspot.com/5354045/diff/1/src/pkg/text/template/set.go#new...
> src/pkg/text/template/set.go:115: err = s.add(tmpl)
> This does not fix the problem. The Set's add method returns an error for
> duplicate template names.  Perhaps this should be:
> 
>   s.tmpl[t.name] = t
>   t.set = s
> 
> The erorr checking in Set's add method is not needed.
> 
> http://codereview.appspot.com/5354045/

It's needed because it's an error, as documented. The bug was that it was
causing a panic.

-rob

[Gus Soden, 2011-11-08 21:59:40]

> It's needed because it's an error, as documented. The bug was that it was
> causing a panic.

The documentation for the Set Parse method says " If a template is redefined,
the element in the set is overwritten with the new definition."   This CL does
not match the documentation, at least as I understand it.

[rsc, 2011-11-08 22:10:17]

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

http://codereview.appspot.com/5354045/diff/1/src/pkg/text/template/set.go#new...
src/pkg/text/template/set.go:104: // to the set.  If a template is redefined,
the element in the set is
This comment suggests that templates can be redefined,
but s.add does not allow that.

[r, 2011-11-08 22:25:06]

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

Please take another look.

[r, 2011-11-08 22:26:08]

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

http://codereview.appspot.com/5354045/diff/1/src/pkg/text/template/set.go#new...
src/pkg/text/template/set.go:104: // to the set.  If a template is redefined,
the element in the set is
On 2011/11/08 22:10:17, rsc wrote:
> This comment suggests that templates can be redefined,
> but s.add does not allow that.

Apologies. I had an unwritten file with an edit to this comment.

[rsc, 2011-11-08 22:27:15]

LGTM

[r, 2011-11-08 22:33:11]

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

text/template: make redefinition of a template in a set more consistent.
Also make it agree with the documentation. You get an error, unless you're
calling Add explicitly, in which case it panics since that's almost certainly
a bug. The discrepancy was caused by a panic that wasn't turned into
an error along one path; deleted the offending function for clarity.

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