code review 4229042: http: set method GET on Get() requests

http: set method GET on Get() requests

[bradfitz, 2011-02-23 22:43:35]

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

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

[adg, 2011-02-23 22:50:34]

LGTM

On 24 February 2011 09:43,  <bradfitz@golang.org> wrote:
> Reviewers: adg,
>
> Message:
> Hello adg (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> http: set method GET on Get() requests
>
> Please review this at http://codereview.appspot.com/4229042/
>
> Affected files:
>  M src/pkg/http/client.go
>  M src/pkg/http/client_test.go
>
>
> Index: src/pkg/http/client.go
> ===================================================================
> --- a/src/pkg/http/client.go
> +++ b/src/pkg/http/client.go
> @@ -171,6 +171,9 @@
>                }
>
>                var req Request
> +               req.Method = "GET"
> +               req.ProtoMajor = 1
> +               req.ProtoMinor = 1
>                if base == nil {
>                        req.URL, err = ParseURL(url)
>                } else {
> Index: src/pkg/http/client_test.go
> ===================================================================
> --- a/src/pkg/http/client_test.go
> +++ b/src/pkg/http/client_test.go
> @@ -8,6 +8,7 @@
>
>  import (
>        "io/ioutil"
> +       "os"
>        "strings"
>        "testing"
>  )
> @@ -38,3 +39,25 @@
>                t.Error("Last-Modified header not found.")
>        }
>  }
> +
> +type recordingTransport struct {
> +       req *Request
> +}
> +
> +func (t *recordingTransport) Do(req *Request) (resp *Response, err
> os.Error) {
> +       t.req = req
> +       return nil, os.NewError("dummy impl")
> +}
> +
> +func TestGetRequestFormat(t *testing.T) {
> +       tr := &recordingTransport{}
> +       client := &Client{transport: tr}
> +       url := "http://www.google.com/"
> +       client.Get(url)
> +       if tr.req.Method != "GET" {
> +               t.Fatalf("expected method %q; got %q", "GET", tr.req.Method)
> +       }
> +       if tr.req.URL.String() != url {
> +               t.Fatalf("expected URL %q; got %q", url,
> tr.req.URL.String())
> +       }
> +}
>
>
>

[bradfitzgoog, 2011-02-23 22:53:39]

I'll update the URL before submitting to make it clear that this test
doesn't hit the network.

http://dummy-url.faketld/no-network should make that more clear.

On Wed, Feb 23, 2011 at 2:49 PM, Andrew Gerrand <adg@golang.org> wrote:

> LGTM
>
> On 24 February 2011 09:43,  <bradfitz@golang.org> wrote:
> > Reviewers: adg,
> >
> > Message:
> > Hello adg (cc: golang-dev@googlegroups.com),
> >
> > I'd like you to review this change to
> > https://go.googlecode.com/hg/
> >
> >
> > Description:
> > http: set method GET on Get() requests
> >
> > Please review this at http://codereview.appspot.com/4229042/
> >
> > Affected files:
> >  M src/pkg/http/client.go
> >  M src/pkg/http/client_test.go
> >
> >
> > Index: src/pkg/http/client.go
> > ===================================================================
> > --- a/src/pkg/http/client.go
> > +++ b/src/pkg/http/client.go
> > @@ -171,6 +171,9 @@
> >                }
> >
> >                var req Request
> > +               req.Method = "GET"
> > +               req.ProtoMajor = 1
> > +               req.ProtoMinor = 1
> >                if base == nil {
> >                        req.URL, err = ParseURL(url)
> >                } else {
> > Index: src/pkg/http/client_test.go
> > ===================================================================
> > --- a/src/pkg/http/client_test.go
> > +++ b/src/pkg/http/client_test.go
> > @@ -8,6 +8,7 @@
> >
> >  import (
> >        "io/ioutil"
> > +       "os"
> >        "strings"
> >        "testing"
> >  )
> > @@ -38,3 +39,25 @@
> >                t.Error("Last-Modified header not found.")
> >        }
> >  }
> > +
> > +type recordingTransport struct {
> > +       req *Request
> > +}
> > +
> > +func (t *recordingTransport) Do(req *Request) (resp *Response, err
> > os.Error) {
> > +       t.req = req
> > +       return nil, os.NewError("dummy impl")
> > +}
> > +
> > +func TestGetRequestFormat(t *testing.T) {
> > +       tr := &recordingTransport{}
> > +       client := &Client{transport: tr}
> > +       url := "http://www.google.com/"
> > +       client.Get(url)
> > +       if tr.req.Method != "GET" {
> > +               t.Fatalf("expected method %q; got %q", "GET",
> tr.req.Method)
> > +       }
> > +       if tr.req.URL.String() != url {
> > +               t.Fatalf("expected URL %q; got %q", url,
> > tr.req.URL.String())
> > +       }
> > +}
> >
> >
> >
>

[bradfitz, 2011-02-23 23:03:35]

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

http: set method GET on Get() requests

R=adg, bradfitzwork
CC=golang-dev
http://codereview.appspot.com/4229042

Committer: Brad Fitzpatrick <bradfitz@golang.org>