code review 6307088: net/http: clarify client return values in docs

net/http: clarify client return values in docs

Also, fixes one violation found during testing where both
response and error could be non-nil when a CheckRedirect test
failed.  This is arguably a minor API (behavior, not
signature) change, but it wasn't documented either way and was
inconsistent & non-Go like.  Any code depending on the old
behavior was wrong anyway.

[bradfitz, 2012-06-13 18:26:20]

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

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

[bradfitz, 2012-06-13 18:29:37]

One sec, the docs are wrong because we're inconsistent on naming the
*http.Response either "r", "res", or "resp".  Changing all to "res" for
consistency.

On Wed, Jun 13, 2012 at 11:26 AM, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> net/http: clarify client return values in docs
>
> Also, fixes one violation found during testing where both
> response and error could be non-nil when a CheckRedirect test
> failed.  This is arguably a minor API (behavior, not
> signature) change, but it wasn't documented either way and was
> inconsistent & non-Go like.  Any code depending on the old
> behavior was wrong anyway.
>
> Please review this at
http://codereview.appspot.com/**6307088/<http://codereview.appspot.com/6307088/>
>
> Affected files:
>  M src/pkg/net/http/client.go
>  M src/pkg/net/http/client_test.**go
>
>
> Index: src/pkg/net/http/client.go
> ==============================**==============================**=======
> --- a/src/pkg/net/http/client.go
> +++ b/src/pkg/net/http/client.go
> @@ -87,6 +87,7 @@
>  // Do sends an HTTP request and returns an HTTP response, following
>  // policy (e.g. redirects, cookies, auth) as configured on the client.
>  //
> +// Either the response or err will be non-nil, but not both.
>  // A non-nil response always contains a non-nil resp.Body.
>  //
>  // Callers should close resp.Body when done reading from it. If
> @@ -151,6 +152,8 @@
>  //    303 (See Other)
>  //    307 (Temporary Redirect)
>  //
> +// Either the response or err will be non-nil, but not both.
> +// A non-nil response always contains a non-nil resp.Body.
>  // Caller should close r.Body when done reading from it.
>  //
>  // Get is a wrapper around DefaultClient.Get.
> @@ -167,6 +170,8 @@
>  //    303 (See Other)
>  //    307 (Temporary Redirect)
>  //
> +// Either the response or err will be non-nil, but not both.
> +// A non-nil response always contains a non-nil resp.Body.
>  // Caller should close r.Body when done reading from it.
>  func (c *Client) Get(url string) (r *Response, err error) {
>        req, err := NewRequest("GET", url, nil)
> @@ -245,12 +250,11 @@
>        }
>
>        method := ireq.Method
> -       err = &url.Error{
> +       return nil, &url.Error{
>                Op:  method[0:1] + strings.ToLower(method[1:]),
>                URL: urlStr,
>                Err: err,
>        }
> -       return
>  }
>
>  func defaultCheckRedirect(req *Request, via []*Request) error {
> Index: src/pkg/net/http/client_test.**go
> ==============================**==============================**=======
> --- a/src/pkg/net/http/client_**test.go
> +++ b/src/pkg/net/http/client_**test.go
> @@ -231,7 +231,6 @@
>
>        checkErr = errors.New("no redirects allowed")
>        res, err = c.Get(ts.URL)
> -       finalUrl = res.Request.URL.String()
>        if e, g := "Get /?n=1: no redirects allowed", fmt.Sprintf("%v",
> err); e != g {
>                t.Errorf("with redirects forbidden, expected error %q, got
> %q", e, g)
>        }
>
>
>

[bradfitz, 2012-06-13 18:33:23]

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

Please take another look.

[bradfitz, 2012-06-13 18:40:43]

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

Please take another look.

[adg, 2012-06-18 15:33:53]

LGTM

[rsc, 2012-06-18 15:39:30]

http://codereview.appspot.com/6307088/diff/9001/src/pkg/net/http/client.go
File src/pkg/net/http/client.go (right):

http://codereview.appspot.com/6307088/diff/9001/src/pkg/net/http/client.go#ne...
src/pkg/net/http/client.go:90: // Either the response or err will be non-nil,
but not both.
This should go without saying. It's the exceptions to this that need
documentation.

http://codereview.appspot.com/6307088/diff/9001/src/pkg/net/http/client.go#ne...
src/pkg/net/http/client.go:91: // A non-nil response always contains a non-nil
res.Body.
Can we please keep this resp? I've never seen 'res' for a response in this code.

http://codereview.appspot.com/6307088/diff/9001/src/pkg/net/http/client.go#ne...
src/pkg/net/http/client.go:155: // Either the response or err will be non-nil,
but not both.
Same.

http://codereview.appspot.com/6307088/diff/9001/src/pkg/net/http/client.go#ne...
src/pkg/net/http/client.go:156: // A non-nil response always contains a non-nil
res.Body.
We needed the disambiguation of resp vs req above; here and below I think r was
just fine. Can we leave them that way?

http://codereview.appspot.com/6307088/diff/9001/src/pkg/net/http/client.go#ne...
src/pkg/net/http/client.go:173: // Either the response or err will be non-nil,
but not both.
Same.

[bradfitz, 2012-06-18 16:02:01]

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

Please take another look.

[rsc, 2012-06-19 04:15:33]

LGTM

[bradfitz, 2012-06-19 16:10:21]

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

net/http: clarify client return values in docs

Also, fixes one violation found during testing where both
response and error could be non-nil when a CheckRedirect test
failed.  This is arguably a minor API (behavior, not
signature) change, but it wasn't documented either way and was
inconsistent & non-Go like.  Any code depending on the old
behavior was wrong anyway.

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