code review 130830043: html/template: don't panic on second execution of unesc...

html/template: don't panic on second execution of unescapable template

Fixes issue 8431.

[adg, 2014-08-18 07:36:44]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[adg, 2014-08-18 07:37:04]

R=r

[r, 2014-08-18 16:00:55]

a little troubled this required adding an import that wasn't needed before. is
there another way?

[adg, 2014-08-18 22:27:21]

On 2014/08/18 16:00:55, r wrote:
> a little troubled this required adding an import that wasn't needed before. is
> there another way?

There are ways to avoid the import:

1. Instead of replacing the "escaped bool" field with the sticky error field,
simply add an extra field. This would remove the need for the sentinel error
value.
2. Implement our own private error type (it's just a couple of lines).

I don't much like 1 because the original impetus for taking this approach was to
avoid further complicating the state of the *Template value.
2 is also fine, but I this package already transitively imports package "errors"
(almost everything does) so I don't really see the point.

[minux, 2014-08-18 22:33:16]

On Mon, Aug 18, 2014 at 6:27 PM, <adg@golang.org> wrote:

> On 2014/08/18 16:00:55, r wrote:
>
>> a little troubled this required adding an import that wasn't needed
>>
> before. is
>
>> there another way?
>>
>
> There are ways to avoid the import:
>
> 1. Instead of replacing the "escaped bool" field with the sticky error
> field, simply add an extra field. This would remove the need for the
> sentinel error value.
> 2. Implement our own private error type (it's just a couple of lines).
>
use fmt.Errof?

>
> I don't much like 1 because the original impetus for taking this
> approach was to avoid further complicating the state of the *Template
> value.
> 2 is also fine, but I this package already transitively imports package
> "errors" (almost everything does) so I don't really see the point.

[adg, 2014-08-18 22:41:37]

I just checked; scan.go already imports "errors". Any reason to avoid doing
so in print.go?


On 19 August 2014 08:32, minux <minux@golang.org> wrote:

>
> On Mon, Aug 18, 2014 at 6:27 PM, <adg@golang.org> wrote:
>
>> On 2014/08/18 16:00:55, r wrote:
>>
>>> a little troubled this required adding an import that wasn't needed
>>>
>> before. is
>>
>>> there another way?
>>>
>>
>> There are ways to avoid the import:
>>
>> 1. Instead of replacing the "escaped bool" field with the sticky error
>> field, simply add an extra field. This would remove the need for the
>> sentinel error value.
>> 2. Implement our own private error type (it's just a couple of lines).
>>
> use fmt.Errof?
>
>>
>> I don't much like 1 because the original impetus for taking this
>> approach was to avoid further complicating the state of the *Template
>> value.
>> 2 is also fine, but I this package already transitively imports package
>> "errors" (almost everything does) so I don't really see the point.
>
>

[r, 2014-08-18 22:50:04]

my concern is not the import per se, but the implication that it's a
new error creation strategy.

i have no idea what scan.go is.

-rob


On Mon, Aug 18, 2014 at 3:41 PM, Andrew Gerrand <adg@golang.org> wrote:
> I just checked; scan.go already imports "errors". Any reason to avoid doing
> so in print.go?
>
>
> On 19 August 2014 08:32, minux <minux@golang.org> wrote:
>>
>>
>> On Mon, Aug 18, 2014 at 6:27 PM, <adg@golang.org> wrote:
>>>
>>> On 2014/08/18 16:00:55, r wrote:
>>>>
>>>> a little troubled this required adding an import that wasn't needed
>>>
>>> before. is
>>>>
>>>> there another way?
>>>
>>>
>>> There are ways to avoid the import:
>>>
>>> 1. Instead of replacing the "escaped bool" field with the sticky error
>>> field, simply add an extra field. This would remove the need for the
>>> sentinel error value.
>>> 2. Implement our own private error type (it's just a couple of lines).
>>
>> use fmt.Errof?
>>>
>>>
>>> I don't much like 1 because the original impetus for taking this
>>> approach was to avoid further complicating the state of the *Template
>>> value.
>>> 2 is also fine, but I this package already transitively imports package
>>> "errors" (almost everything does) so I don't really see the point.
>
>

[adg, 2014-08-18 22:51:58]

On 19 August 2014 08:49, Rob Pike <r@golang.org> wrote:

> my concern is not the import per se, but the implication that it's a
> new error creation strategy.
>

So it's the use of the sentinel value you are concerned with?


>  i have no idea what scan.go is.
>

Another file in package fmt.

[adg, 2014-08-18 22:52:38]

On 19 August 2014 08:51, Andrew Gerrand <adg@golang.org> wrote:

> Another file in package fmt.


That'll learn me for working on two CLs at once. I was wrong

[r, 2014-08-18 22:57:47]

no, it's what i said: a new strategy for creating errors.

i should look, but: what does the package do now when it needs to
create an error value? the arrival of "errors" means it probably does
this some other way.

-rob


On Mon, Aug 18, 2014 at 3:52 PM, Andrew Gerrand <adg@golang.org> wrote:
>
> On 19 August 2014 08:51, Andrew Gerrand <adg@golang.org> wrote:
>>
>> Another file in package fmt.
>
>
> That'll learn me for working on two CLs at once. I was wrong

[adg, 2014-08-18 23:00:40]

fmt.Errorf.


On 19 August 2014 08:57, Rob Pike <r@golang.org> wrote:

> no, it's what i said: a new strategy for creating errors.
>
> i should look, but: what does the package do now when it needs to
> create an error value? the arrival of "errors" means it probably does
> this some other way.
>
> -rob
>
>
> On Mon, Aug 18, 2014 at 3:52 PM, Andrew Gerrand <adg@golang.org> wrote:
> >
> > On 19 August 2014 08:51, Andrew Gerrand <adg@golang.org> wrote:
> >>
> >> Another file in package fmt.
> >
> >
> > That'll learn me for working on two CLs at once. I was wrong
>

[adg, 2014-08-19 01:37:11]

PTAL

I switched to fmt.Errorf

[r, 2014-08-19 03:52:51]

LGTM

[adg, 2014-08-19 04:24:23]

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

html/template: don't panic on second execution of unescapable template

Fixes issue 8431.

LGTM=r
R=golang-codereviews, r, minux
CC=golang-codereviews
https://codereview.appspot.com/130830043