code review 4535056: http: remove finalURL from Client.Get; move to Response

http: remove finalURL from Client.Get; move to Response

This CL:

-- removes Response.RequestMethod string
-- adds Response.Request *Request
-- removes the finalURL result parameter from client.Get()
-- adds a gofix rule for callers of http.Get which assign
   the final url to the blank identifier; warning otherwise

Caller who did:

res, finalURL, err := http.Get(...)

now need to do:

res, err := http.Get(...)
if err != nil {
   ...
}
finalURL := res.Request.URL.String()

[bradfitz, 2011-05-12 21:15:13]

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

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

[rsc, 2011-05-12 21:16:47]

Please add a gofix module that does

a, _, c = http.Get(...)  ->  a, c = http.Get(...)

and

a, b, c = http.Get(...) -> warn about it

Thanks.

[bradfitz, 2011-05-12 22:18:59]

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

Please take another look.

[bradfitz, 2011-05-12 23:06:32]

Now with a gofixer.

On Thu, May 12, 2011 at 3:18 PM, <bradfitz@golang.org> wrote:

> Hello rsc@golang.org (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
> http://codereview.appspot.com/4535056/
>

[rsc, 2011-05-13 02:45:13]

LGTM

http://codereview.appspot.com/4535056/diff/7001/src/cmd/gofix/httpfinalurl.go
File src/cmd/gofix/httpfinalurl.go (right):

http://codereview.appspot.com/4535056/diff/7001/src/cmd/gofix/httpfinalurl.go...
src/cmd/gofix/httpfinalurl.go:40: assign, ok := n.(*ast.AssignStmt)
s/assign/as/ if you want

http://codereview.appspot.com/4535056/diff/7001/src/cmd/gofix/httpfinalurl.go...
src/cmd/gofix/httpfinalurl.go:41: if !ok || len(assign.Lhs) != 3 {
if !ok || len(as.Lhs) != 3 || len(as.Rhs) != 1 {

http://codereview.appspot.com/4535056/diff/7001/src/cmd/gofix/httpfinalurl.go...
src/cmd/gofix/httpfinalurl.go:45: call, ok := assign.Rhs[0].(*ast.CallExpr)
if !isCall(as.Rhs[0], "http", "Get") {

http://codereview.appspot.com/4535056/diff/7001/src/cmd/gofix/httpfinalurl.go...
src/cmd/gofix/httpfinalurl.go:53: ident, ok := assign.Lhs[1].(*ast.Ident)
You don't want to return here.  We know it needs fixing.

if isBlank(assign.Lhs[1]) {
    as.Lhs = []ast.Expr{as.Lhs[0], as.Lhs[2]}
    fixed = true
} else {
    warn(as.Pos(), "call to http.Get records final URL")
}

http://codereview.appspot.com/4535056/diff/7001/src/cmd/gofix/httpfinalurl_te...
File src/cmd/gofix/httpfinalurl_test.go (right):

http://codereview.appspot.com/4535056/diff/7001/src/cmd/gofix/httpfinalurl_te...
src/cmd/gofix/httpfinalurl_test.go:22: resp = resp
_, _ = resp, err

http://codereview.appspot.com/4535056/diff/7001/src/pkg/http/response.go
File src/pkg/http/response.go (right):

http://codereview.appspot.com/4535056/diff/7001/src/pkg/http/response.go#newc...
src/pkg/http/response.go:68: // The most recent Request that was sent to obtain
this Response.
s/most recent //

(The earlier requests had other responses.)

[bradfitz, 2011-05-13 14:14:17]

All done.

On Thu, May 12, 2011 at 7:45 PM, <rsc@golang.org> wrote:

> LGTM
>
>
>
>
> http://codereview.appspot.com/4535056/diff/7001/src/cmd/gofix/httpfinalurl.go
> File src/cmd/gofix/httpfinalurl.go (right):
>
>
>
http://codereview.appspot.com/4535056/diff/7001/src/cmd/gofix/httpfinalurl.go...
> src/cmd/gofix/httpfinalurl.go:40: assign, ok := n.(*ast.AssignStmt)
> s/assign/as/ if you want
>
>
>
http://codereview.appspot.com/4535056/diff/7001/src/cmd/gofix/httpfinalurl.go...
> src/cmd/gofix/httpfinalurl.go:41: if !ok || len(assign.Lhs) != 3 {
> if !ok || len(as.Lhs) != 3 || len(as.Rhs) != 1 {
>
>
>
http://codereview.appspot.com/4535056/diff/7001/src/cmd/gofix/httpfinalurl.go...
> src/cmd/gofix/httpfinalurl.go:45: call, ok :=
> assign.Rhs[0].(*ast.CallExpr)
> if !isCall(as.Rhs[0], "http", "Get") {
>
>
>
http://codereview.appspot.com/4535056/diff/7001/src/cmd/gofix/httpfinalurl.go...
> src/cmd/gofix/httpfinalurl.go:53: ident, ok :=
> assign.Lhs[1].(*ast.Ident)
> You don't want to return here.  We know it needs fixing.
>
> if isBlank(assign.Lhs[1]) {
>    as.Lhs = []ast.Expr{as.Lhs[0], as.Lhs[2]}
>    fixed = true
> } else {
>    warn(as.Pos(), "call to http.Get records final URL")
> }
>
>
>
http://codereview.appspot.com/4535056/diff/7001/src/cmd/gofix/httpfinalurl_te...
> File src/cmd/gofix/httpfinalurl_test.go (right):
>
>
>
http://codereview.appspot.com/4535056/diff/7001/src/cmd/gofix/httpfinalurl_te...
> src/cmd/gofix/httpfinalurl_test.go:22: resp = resp
> _, _ = resp, err
>
> http://codereview.appspot.com/4535056/diff/7001/src/pkg/http/response.go
> File src/pkg/http/response.go (right):
>
>
>
http://codereview.appspot.com/4535056/diff/7001/src/pkg/http/response.go#newc...
> src/pkg/http/response.go:68: // The most recent Request that was sent to
> obtain this Response.
> s/most recent //
>
> (The earlier requests had other responses.)
>
>
> http://codereview.appspot.com/4535056/
>

[bradfitz, 2011-05-13 14:31:35]

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

http: remove finalURL from Client.Get; move to Response

This CL:

-- removes Response.RequestMethod string
-- adds Response.Request *Request
-- removes the finalURL result parameter from client.Get()
-- adds a gofix rule for callers of http.Get which assign
   the final url to the blank identifier; warning otherwise

Caller who did:

res, finalURL, err := http.Get(...)

now need to do:

res, err := http.Get(...)
if err != nil {
   ...
}
finalURL := res.Request.URL.String()

R=rsc
CC=golang-dev
http://codereview.appspot.com/4535056