code review 5026041: exp/template/html: moved error docs out of package docs...

exp/template/html: moved error docs out of package docs onto error codes

This replaces the errStr & errLine members of context with a single err
*Error, and introduces a number of const error codes, one per
escape-time failure mode, that can be separately documented.

The changes to the error documentation moved from doc.go to error.go
are cosmetic.

[MikeSamuel, 2011-09-15 00:50:33]

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

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

[r, 2011-09-15 01:03:45]

http://codereview.appspot.com/5026041/diff/5001/src/pkg/exp/template/html/err...
File src/pkg/exp/template/html/error.go (right):

http://codereview.appspot.com/5026041/diff/5001/src/pkg/exp/template/html/err...
src/pkg/exp/template/html/error.go:35: //   it from filtering.
i filed a bug with godoc. once it's fixed this comment will appear in godoc
output.

http://codereview.appspot.com/5026041/diff/5001/src/pkg/exp/template/html/err...
src/pkg/exp/template/html/error.go:62: //   run-time value of {{.C}}.
didn't i have a much shorter version of this? this is too long.

http://codereview.appspot.com/5026041/diff/5001/src/pkg/exp/template/html/esc...
File src/pkg/exp/template/html/escape.go (right):

http://codereview.appspot.com/5026041/diff/5001/src/pkg/exp/template/html/esc...
src/pkg/exp/template/html/escape.go:480: args, f = append([]interface{}{e.name,
line}, args...), "%s:%d: "+f
you don't need this; it's easy:
return fmt.Errorf("%s:%d: "+f, e.name, line, args...)

[nigeltao, 2011-09-15 01:33:31]

I think that the ErrXxx types need to be a non-interface type (i.e. a struct or
string), otherwise you can't reliably type switch between them.

See the coincidentally recent "question about switch on (type)" thread on
golang-nuts:
http://groups.google.com/group/golang-nuts/browse_thread/thread/b0fe1cc338f4317c

http://codereview.appspot.com/5026041/diff/5001/src/pkg/exp/template/html/err...
File src/pkg/exp/template/html/error.go (right):

http://codereview.appspot.com/5026041/diff/5001/src/pkg/exp/template/html/err...
src/pkg/exp/template/html/error.go:52: ErrAmbigContext os.Error
I'm coming in late to this CL, and I don't know how it plays with godoc, but I'd
add a blank line afterwards to break up this enormous chunk of text. Similarly
after each type below.

[r, 2011-09-15 01:43:11]

http://codereview.appspot.com/5026041/diff/5001/src/pkg/exp/template/html/err...
File src/pkg/exp/template/html/error.go (right):

http://codereview.appspot.com/5026041/diff/5001/src/pkg/exp/template/html/err...
src/pkg/exp/template/html/error.go:105: ErrBranchEnd os.Error
ack! yes. i missed this, conflating these with the JS and CSS things. you can't
type switch usefully on interface types. (see my recent blog post on
golang.org). these need to be concrete types. they can be just strings, as long
as they implement String()

thinking out loud and maybe stupidly, you could do

type errBase string
func (e ErrBase) String() string {
   return string(e)
}

and then

type ErrBranchEnd struct { errBase }

that saves writing out the String method each time, but it's hardly a major
saving. they could just be strings too.

[nigeltao, 2011-09-15 01:44:03]

A broader question is why are these errors separate types? It's not like a user
of this package is going to do different things depending on whether the error
return is an ErrPartialCharset or an ErrPartialEscape. Either way, the template
is not HTML-escapable, and the reason why is already captured by err.String().

I think it's perfectly laudable to make the package's error messages consistent,
and well documented, but I don't think that that requires a separate Go type for
each type of error. Instead, have all of the error messages look like
"htmltemplate: partial charset: foobar" and have the string "partial charset" in
the error section of the package documentation.

This is just my immediate reaction, though. I might need to think a bit more
about it.

[nigeltao, 2011-09-15 01:50:05]

One possibility is

type Error struct {
  ErrorCode ErrorCode
  Line      int
  Detail    string
}

func (e *Error) String() {
  return fmt.Sprintf("htmltemplate: %v at line %d: %s", e.ErrorCode, e.Line,
e.Detail)
}

type ErrorCode int

const (
  OK ErrorCode = iota
  ErrAmbiguousContext
  ErrBranchEnd
  ErrEndContext
)

func (e ErrorCode) String() {
  // look up a [...]
}

[MikeSamuel, 2011-09-15 01:54:43]

On 2011/09/15 01:50:05, nigeltao wrote:
> One possibility is
> 
> type Error struct {
>   ErrorCode ErrorCode

I like this.  Latest snapshot does not have it but will implement.

http://codereview.appspot.com/5026041/diff/5001/src/pkg/exp/template/html/err...
File src/pkg/exp/template/html/error.go (right):

http://codereview.appspot.com/5026041/diff/5001/src/pkg/exp/template/html/err...
src/pkg/exp/template/html/error.go:52: ErrAmbigContext os.Error
On 2011/09/15 01:33:32, nigeltao wrote:
> I'm coming in late to this CL, and I don't know how it plays with godoc, but
I'd
> add a blank line afterwards to break up this enormous chunk of text. Similarly
> after each type below.

Done.

http://codereview.appspot.com/5026041/diff/5001/src/pkg/exp/template/html/err...
src/pkg/exp/template/html/error.go:62: //   run-time value of {{.C}}.
On 2011/09/15 01:03:45, r wrote:
> didn't i have a much shorter version of this? this is too long.

You did,  I misinterpreted

"[r: i think this is too much detail. i’ve distilled it up top but left this
here for reference"

and left the bracketed content in.  Removed.

http://codereview.appspot.com/5026041/diff/5001/src/pkg/exp/template/html/err...
src/pkg/exp/template/html/error.go:105: ErrBranchEnd os.Error
On 2011/09/15 01:43:11, r wrote:
> ack! yes. i missed this, conflating these with the JS and CSS things. you
can't
> type switch usefully on interface types. (see my recent blog post on
> http://golang.org). these need to be concrete types. they can be just strings,
as long
> as they implement String()
> 
> thinking out loud and maybe stupidly, you could do
> 
> type errBase string
> func (e ErrBase) String() string {
>    return string(e)
> }
> 
> and then
> 
> type ErrBranchEnd struct { errBase }
> 
> that saves writing out the String method each time, but it's hardly a major
> saving. they could just be strings too.

I just made them a string.

http://codereview.appspot.com/5026041/diff/5001/src/pkg/exp/template/html/esc...
File src/pkg/exp/template/html/escape.go (right):

http://codereview.appspot.com/5026041/diff/5001/src/pkg/exp/template/html/esc...
src/pkg/exp/template/html/escape.go:480: args, f = append([]interface{}{e.name,
line}, args...), "%s:%d: "+f
On 2011/09/15 01:03:45, r wrote:
> you don't need this; it's easy:
> return fmt.Errorf("%s:%d: "+f, e.name, line, args...)

That doesn't seem to work.

escape.go:480: too many arguments in call to fmt.Errorf

[MikeSamuel, 2011-09-15 02:17:47]

On 2011/09/15 01:50:05, nigeltao wrote:
> One possibility is
> 
> type Error struct {
>   ErrorCode ErrorCode
>   Line      int
>   Detail    string
> }
> 
> func (e *Error) String() {
>   return fmt.Sprintf("htmltemplate: %v at line %d: %s", e.ErrorCode, e.Line,
> e.Detail)
> }
> 
> type ErrorCode int
> 
> const (
>   OK ErrorCode = iota
>   ErrAmbiguousContext
>   ErrBranchEnd
>   ErrEndContext
> )
> 
> func (e ErrorCode) String() {
>   // look up a [...]
> }

Done.  ErrorCode is called ErrorKind because I saw that name in another package
I think.

[MikeSamuel, 2011-09-15 03:17:38]

> Done.  ErrorCode is called ErrorKind because I saw that name in another
package
> I think.

grep says I was wrong.
Renamed ErrorKind -> ErrorCode.

[nigeltao, 2011-09-15 08:28:42]

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/er...
File src/pkg/exp/template/html/error.go (right):

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/er...
src/pkg/exp/template/html/error.go:20: s string
You might as well export all the fields: s/s/Description/.

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/er...
src/pkg/exp/template/html/error.go:24: type ErrorCode uint8
I'd s/uint8/int/. The space saving isn't important.

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/er...
src/pkg/exp/template/html/error.go:185: return fmt.Sprintf("%s:%d: %s", e.Name,
e.Line, e.s)
I'd like to see the package name in here somewhere.
http://golang.org/doc/effective_go.html#errors

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/es...
File src/pkg/exp/template/html/escape.go (right):

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/es...
src/pkg/exp/template/html/escape.go:35: func EscapeSet(s *template.Set, names
...string) (*template.Set, *Error) {
I'd return an os.Error, not an *Error.

Note that a nil *Error is a non-nil os.Error. With what you have here, people
could do
func foo() os.Error {
  _, err := html.EscapeSet(etc)
  return err
}
and wonder why their os.Error is non-nil.

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/es...
src/pkg/exp/template/html/escape.go:484: func (e *escaper) errorf(f string, k
ErrorCode, line int, args ...interface{}) *Error {
Why is this a method of escaper? I'd make it a function and move it to error.go.

I'd also order the arguments (e ErrorCode, lint int, format string, args
...interface{}).

What you have now looks weird at the call site where the format has one "%s",
say, but three args after it.

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/tr...
File src/pkg/exp/template/html/transition.go (right):

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/tr...
src/pkg/exp/template/html/transition.go:210: err:   &Error{ErrorCode:
ErrSlashAmbig, s: fmt.Sprintf("'/' could start div or regexp: %.32q", s[i:])},
Use errorf?

[r, 2011-09-15 13:02:10]

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/do...
File src/pkg/exp/template/html/doc.go (right):

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/do...
src/pkg/exp/template/html/doc.go:72: Source file error.go contains error types
and each type's documentation is
it's odd to mention a source file here.  maybe just

The various error conditions are illustrated by an example....

that's a sufficient cross reference.  (look at the godoc output to see how it
stitches together. remember these comments are for people reading godoc more
than for people reading the code.)

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/er...
File src/pkg/exp/template/html/error.go (right):

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/er...
src/pkg/exp/template/html/error.go:27: // this package may also fail at runtime,
or panic.
calls to Escape or EscapeSet may also panic when given certain forms of invalid
input, documented below.

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/er...
src/pkg/exp/template/html/error.go:88: //   Templates that end without closing
tags will trigger this warning.
s/warning/error/

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/er...
src/pkg/exp/template/html/error.go:112: //  whole comment as a type string (see
below).
type string?
and what does 'below' refer to? don't depend on ordering of output in godoc.

[MikeSamuel, 2011-09-15 15:59:53]

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/do...
File src/pkg/exp/template/html/doc.go (right):

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/do...
src/pkg/exp/template/html/doc.go:72: Source file error.go contains error types
and each type's documentation is
On 2011/09/15 13:02:10, r wrote:
> it's odd to mention a source file here.  maybe just
> 
> The various error conditions are illustrated by an example....
> 
> that's a sufficient cross reference.  (look at the godoc output to see how it
> stitches together. remember these comments are for people reading godoc more
> than for people reading the code.)

Done.

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/er...
File src/pkg/exp/template/html/error.go (right):

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/er...
src/pkg/exp/template/html/error.go:20: s string
On 2011/09/15 08:28:42, nigeltao wrote:
> You might as well export all the fields: s/s/Description/.

Done.

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/er...
src/pkg/exp/template/html/error.go:24: type ErrorCode uint8
On 2011/09/15 08:28:42, nigeltao wrote:
> I'd s/uint8/int/. The space saving isn't important.

Done.

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/er...
src/pkg/exp/template/html/error.go:27: // this package may also fail at runtime,
or panic.
On 2011/09/15 13:02:10, r wrote:
> calls to Escape or EscapeSet may also panic when given certain forms of
invalid
> input, documented below.

Rewrote.  Did not use that text as there are no panics left.

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/er...
src/pkg/exp/template/html/error.go:88: //   Templates that end without closing
tags will trigger this warning.
On 2011/09/15 13:02:10, r wrote:
> s/warning/error/

Done.

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/er...
src/pkg/exp/template/html/error.go:112: //  whole comment as a type string (see
below).
On 2011/09/15 13:02:10, r wrote:
> type string?
> and what does 'below' refer to? don't depend on ordering of output in godoc.

Rewrote.

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/er...
src/pkg/exp/template/html/error.go:185: return fmt.Sprintf("%s:%d: %s", e.Name,
e.Line, e.s)
On 2011/09/15 08:28:42, nigeltao wrote:
> I'd like to see the package name in here somewhere.
> http://golang.org/doc/effective_go.html#errors

Done.

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/es...
File src/pkg/exp/template/html/escape.go (right):

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/es...
src/pkg/exp/template/html/escape.go:35: func EscapeSet(s *template.Set, names
...string) (*template.Set, *Error) {
On 2011/09/15 08:28:42, nigeltao wrote:
> I'd return an os.Error, not an *Error.
> 
> Note that a nil *Error is a non-nil os.Error. With what you have here, people
> could do
> func foo() os.Error {
>   _, err := html.EscapeSet(etc)
>   return err
> }
> and wonder why their os.Error is non-nil.

Done.

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/es...
src/pkg/exp/template/html/escape.go:484: func (e *escaper) errorf(f string, k
ErrorCode, line int, args ...interface{}) *Error {
On 2011/09/15 08:28:42, nigeltao wrote:
> Why is this a method of escaper? I'd make it a function and move it to
error.go.

Done.  I originally had it as a method of escaper because escaper was carrying
name information but now it uses nothing from escaper.

> I'd also order the arguments (e ErrorCode, lint int, format string, args
> ...interface{}).
> 
> What you have now looks weird at the call site where the format has one "%s",
> say, but three args after it.

Done.

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/tr...
File src/pkg/exp/template/html/transition.go (right):

http://codereview.appspot.com/5026041/diff/15003/src/pkg/exp/template/html/tr...
src/pkg/exp/template/html/transition.go:210: err:   &Error{ErrorCode:
ErrSlashAmbig, s: fmt.Sprintf("'/' could start div or regexp: %.32q", s[i:])},
On 2011/09/15 08:28:42, nigeltao wrote:
> Use errorf?

Done here and elsewhere within this file.

[r, 2011-09-15 19:33:14]

LGTM
leaving for nigeltao

http://codereview.appspot.com/5026041/diff/11003/src/pkg/exp/template/html/do...
File src/pkg/exp/template/html/doc.go (right):

http://codereview.appspot.com/5026041/diff/11003/src/pkg/exp/template/html/do...
src/pkg/exp/template/html/doc.go:73: explanation of the problem.
See the documentation of ErrorCode for details.

http://codereview.appspot.com/5026041/diff/11003/src/pkg/exp/template/html/do...
src/pkg/exp/template/html/doc.go:82: 
delete at least one of these blank lines.

http://codereview.appspot.com/5026041/diff/11003/src/pkg/exp/template/html/er...
File src/pkg/exp/template/html/error.go (right):

http://codereview.appspot.com/5026041/diff/11003/src/pkg/exp/template/html/er...
src/pkg/exp/template/html/error.go:11: // Error is a problem encountered during
template Escaping.
s/is/describes/ ?

[MikeSamuel, 2011-09-15 20:13:49]

Incremental diffs at
http://codereview.appspot.com/5026041/diff2/11003:28001/src/pkg/exp/template/...

http://codereview.appspot.com/5026041/diff/11003/src/pkg/exp/template/html/do...
File src/pkg/exp/template/html/doc.go (right):

http://codereview.appspot.com/5026041/diff/11003/src/pkg/exp/template/html/do...
src/pkg/exp/template/html/doc.go:73: explanation of the problem.
On 2011/09/15 19:33:14, r wrote:
> See the documentation of ErrorCode for details.

Done.

http://codereview.appspot.com/5026041/diff/11003/src/pkg/exp/template/html/do...
src/pkg/exp/template/html/doc.go:82: 
On 2011/09/15 19:33:14, r wrote:
> delete at least one of these blank lines.

Done.  Only two blank lines before section headers now.

http://codereview.appspot.com/5026041/diff/11003/src/pkg/exp/template/html/er...
File src/pkg/exp/template/html/error.go (right):

http://codereview.appspot.com/5026041/diff/11003/src/pkg/exp/template/html/er...
src/pkg/exp/template/html/error.go:11: // Error is a problem encountered during
template Escaping.
On 2011/09/15 19:33:14, r wrote:
> s/is/describes/ ?

Done.

[nigeltao, 2011-09-16 00:46:18]

LGTM.

http://codereview.appspot.com/5026041/diff/28001/src/pkg/exp/template/html/er...
File src/pkg/exp/template/html/error.go (right):

http://codereview.appspot.com/5026041/diff/28001/src/pkg/exp/template/html/er...
src/pkg/exp/template/html/error.go:26: // We define types for each error that
manifests while escaping templates, but
s/types/codes/.

http://codereview.appspot.com/5026041/diff/28001/src/pkg/exp/template/html/er...
src/pkg/exp/template/html/error.go:29: // Error: "ZgotmplZ"
This isn't really an error, though, in that html.EscapeSet will return a nil
os.Error. Maybe reword this section so that that's clearer. I might take a crack
at that in a separate CL.

http://codereview.appspot.com/5026041/diff/28001/src/pkg/exp/template/html/er...
src/pkg/exp/template/html/error.go:143: // Error: "unfinished JS regexp charset
in ..."
s/Error/ErrPartialCharset/. Similarly at line 151.

http://codereview.appspot.com/5026041/diff/28001/src/pkg/exp/template/html/es...
File src/pkg/exp/template/html/escape.go (right):

http://codereview.appspot.com/5026041/diff/28001/src/pkg/exp/template/html/es...
src/pkg/exp/template/html/escape.go:292: func (e *escaper) join(a, b context,
line int, nodeName string) context {
join no longer needs to be a method of escaper.

http://codereview.appspot.com/5026041/diff/28001/src/pkg/exp/template/html/es...
src/pkg/exp/template/html/escape.go:340: c0.err.Description, c0.err.Line =
fmt.Sprintf("on range loop re-entry: %s", c0.err.Description), n.Line
I'd split these two assignments over two lines.

[MikeSamuel, 2011-09-16 02:05:40]

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

exp/template/html: moved error docs out of package docs onto error codes

This replaces the errStr & errLine members of context with a single err
*Error, and introduces a number of const error codes, one per
escape-time failure mode, that can be separately documented.

The changes to the error documentation moved from doc.go to error.go
are cosmetic.

R=r, nigeltao
CC=golang-dev
http://codereview.appspot.com/5026041

http://codereview.appspot.com/5026041/diff/28001/src/pkg/exp/template/html/er...
File src/pkg/exp/template/html/error.go (right):

http://codereview.appspot.com/5026041/diff/28001/src/pkg/exp/template/html/er...
src/pkg/exp/template/html/error.go:26: // We define types for each error that
manifests while escaping templates, but
On 2011/09/16 00:46:18, nigeltao wrote:
> s/types/codes/.

Done.

http://codereview.appspot.com/5026041/diff/28001/src/pkg/exp/template/html/er...
src/pkg/exp/template/html/error.go:29: // Error: "ZgotmplZ"
On 2011/09/16 00:46:18, nigeltao wrote:
> This isn't really an error, though, in that html.EscapeSet will return a nil
> os.Error. Maybe reword this section so that that's clearer. I might take a
crack
> at that in a separate CL.

Reworded.

http://codereview.appspot.com/5026041/diff/28001/src/pkg/exp/template/html/er...
src/pkg/exp/template/html/error.go:143: // Error: "unfinished JS regexp charset
in ..."
On 2011/09/16 00:46:18, nigeltao wrote:
> s/Error/ErrPartialCharset/. Similarly at line 151.

Done.

http://codereview.appspot.com/5026041/diff/28001/src/pkg/exp/template/html/es...
File src/pkg/exp/template/html/escape.go (right):

http://codereview.appspot.com/5026041/diff/28001/src/pkg/exp/template/html/es...
src/pkg/exp/template/html/escape.go:292: func (e *escaper) join(a, b context,
line int, nodeName string) context {
On 2011/09/16 00:46:18, nigeltao wrote:
> join no longer needs to be a method of escaper.

Done.

http://codereview.appspot.com/5026041/diff/28001/src/pkg/exp/template/html/es...
src/pkg/exp/template/html/escape.go:340: c0.err.Description, c0.err.Line =
fmt.Sprintf("on range loop re-entry: %s", c0.err.Description), n.Line
On 2011/09/16 00:46:18, nigeltao wrote:
> I'd split these two assignments over two lines.

Done.