Code review comment for lp:~jtv/gwacl/http-error

Revision history for this message
Raphaƫl Badin (rvb) wrote :

Looks good, a very nice improvement!

[0]

28 +// HTTPError is an extended version of the standard "error" interface. It
29 +// adds an HTTP status code.
30 +type HTTPError interface {
31 + error
32 + Status() int
33 +}

For clarity, I'd be in favor of renaming Status() into StatusCode().

[1]

253 + c.Check(err, ErrorMatches, ".*102.*")
254 + c.Check(err, ErrorMatches, ".*Frotzed.*")

and

265 + c.Check(err, ErrorMatches, ".*102.*")
266 + c.Check(err, ErrorMatches, ".*Frotzed.*")

and

277 + c.Check(err, ErrorMatches, ".*102.*")
278 + c.Check(err, ErrorMatches, ".*Frotzed.*")

and

289 + c.Check(err, ErrorMatches, ".*146.*")
290 + c.Check(err, ErrorMatches, ".*Frotzed.*")

and

300 + c.Check(err, ErrorMatches, ".*246.*")
301 + c.Check(err, ErrorMatches, ".*Frotzed.*")

I think you should fold these into on statement like this:
c.Check(err, ErrorMatches, ".*102 Frotzed.*")

because the status of the response is "102 Frotzed":

    response := &http.Response{
        Status: "102 Frotzed",
        StatusCode: 102,
        Body: ioutil.NopCloser(strings.NewReader("<Error><Code>Frotzed</Code><Message>failed to put blob</Message></Error>")),
    }

review: Approve

« Back to merge proposal