code review 626042: http: add Error helper function

http: add Error helper function

[adg, 2010-03-19 01:13:12]

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

I'd like you to review this change.

[rsc, 2010-03-19 01:15:20]

s/a specified/the specified/.

You say code and error message in the doc comment
but error message and code in the signature.
I think the doc comment is right and the func should change.

Russ


On Thu, Mar 18, 2010 at 18:13,  <adg@golang.org> wrote:
> Reviewers: r, rsc,
>
> Message:
> Hello r, rsc (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change.
>
>
> Description:
> http: add Error helper function
>
> Please review this at http://codereview.appspot.com/626042/show
>
> Affected files:
>  M src/pkg/http/server.go
>
>
> Index: src/pkg/http/server.go
> ===================================================================
> --- a/src/pkg/http/server.go
> +++ b/src/pkg/http/server.go
> @@ -339,6 +339,13 @@
>  // that replies to each request with a ``404 page not found'' reply.
>  func NotFoundHandler() Handler { return HandlerFunc(NotFound) }
>
> +// Error replies to the request with a specified HTTP code and error
> message.
> +func Error(c *Conn, error string, code int) {
> +       c.SetHeader("Content-Type", "text/plain; charset=utf-8")
> +       c.WriteHeader(code)
> +       io.WriteString(c, error)
> +}
> +
>  // Redirect replies to the request with a redirect to url,
>  // which may be a path relative to the request path.
>  func Redirect(c *Conn, url string, code int) {
>
>
>

[adg, 2010-03-19 01:17:25]

On 19 March 2010 12:15, Russ Cox <rsc@golang.org> wrote:
> s/a specified/the specified/.
>
> You say code and error message in the doc comment
> but error message and code in the signature.
> I think the doc comment is right and the func should change.

It then becomes inconsistent with Redirect.

> Russ
>
>
> On Thu, Mar 18, 2010 at 18:13,  <adg@golang.org> wrote:
>> Reviewers: r, rsc,
>>
>> Message:
>> Hello r, rsc (cc: golang-dev@googlegroups.com),
>>
>> I'd like you to review this change.
>>
>>
>> Description:
>> http: add Error helper function
>>
>> Please review this at http://codereview.appspot.com/626042/show
>>
>> Affected files:
>>  M src/pkg/http/server.go
>>
>>
>> Index: src/pkg/http/server.go
>> ===================================================================
>> --- a/src/pkg/http/server.go
>> +++ b/src/pkg/http/server.go
>> @@ -339,6 +339,13 @@
>>  // that replies to each request with a ``404 page not found'' reply.
>>  func NotFoundHandler() Handler { return HandlerFunc(NotFound) }
>>
>> +// Error replies to the request with a specified HTTP code and error
>> message.
>> +func Error(c *Conn, error string, code int) {
>> +       c.SetHeader("Content-Type", "text/plain; charset=utf-8")
>> +       c.WriteHeader(code)
>> +       io.WriteString(c, error)
>> +}
>> +
>>  // Redirect replies to the request with a redirect to url,
>>  // which may be a path relative to the request path.
>>  func Redirect(c *Conn, url string, code int) {
>>
>>
>>
>

[adg, 2010-03-19 01:26:45]

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

Please take another look.

[r, 2010-03-19 17:37:57]

LGTM but rsc owns this code

[rsc, 2010-03-19 18:03:49]

LGTM

fix and submit

http://codereview.appspot.com/626042/diff/10001/11001
File src/pkg/http/server.go (right):

http://codereview.appspot.com/626042/diff/10001/11001#newcode335
src/pkg/http/server.go:335: io.WriteString(c, error)
Please make this fmt.Println(c, error)
so that the newline is not required.

http://codereview.appspot.com/626042/diff/10001/11001#newcode339
src/pkg/http/server.go:339: func NotFound(c *Conn, req *Request) { Error(c, "404
page not found\n", StatusNotFound) }
s/\\n//

[r2, 2010-03-19 18:15:53]

On Mar 19, 2010, at 11:03 AM, rsc@golang.org wrote:

> LGTM
>
> fix and submit
>
>
>
> http://codereview.appspot.com/626042/diff/10001/11001
> File src/pkg/http/server.go (right):
>
> http://codereview.appspot.com/626042/diff/10001/11001#newcode335
> src/pkg/http/server.go:335: io.WriteString(c, error)
> Please make this fmt.Println(c, error)

fmt.Fprintln he means.

> so that the newline is not required.
>
> http://codereview.appspot.com/626042/diff/10001/11001#newcode339
> src/pkg/http/server.go:339: func NotFound(c *Conn, req *Request) {
> Error(c, "404 page not found\n", StatusNotFound) }
> s/\\n//
>
> http://codereview.appspot.com/626042/show

[adg, 2010-03-19 23:22:13]

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

http: add Error helper function

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