Code review comment for lp:~themue/juju-core/012-local-storage

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

Overall LGTM, modulo some trivial suggestions.

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go
File environs/local/storage.go (right):

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newcode11
environs/local/storage.go:11:
d

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newcode14
environs/local/storage.go:14:
var _ environ.Storage = (*storage)(nil) ?

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newcode30
environs/local/storage.go:30: // exist, it should return a
*NotFoundError.
s/*NotFoundError/NotFoundError - or even use roger's NotFoundf() ?

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newcode68
environs/local/storage.go:68: if string(body) == "" {
len(body) == 0 ?

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newcode76
environs/local/storage.go:76: // URL returns a URL that can be used to
access the given storage file.
s/a URL/an URL

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newcode82
environs/local/storage.go:82: // The length must give the total length
of the file.
s/must give/must be set to/

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newcode98
environs/local/storage.go:98: return errors.New(resp.Status)
Include the StatusCode in the error text?

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newcode120
environs/local/storage.go:120: return errors.New(resp.Status)
as above

https://codereview.appspot.com/7346052/diff/1/environs/local/storage_test.go
File environs/local/storage_test.go (right):

https://codereview.appspot.com/7346052/diff/1/environs/local/storage_test.go#newcode70
environs/local/storage_test.go:70: c.Assert(err, IsNil)
if it's check*, use c.Check, rather than c.Assert - like you're doing
above. Otherwise, call it assert*

https://codereview.appspot.com/7346052/diff/1/environs/local/storage_test.go#newcode83
environs/local/storage_test.go:83: for a := attempt.Start(); a.Next(); {
If you're not retrying (attempt strategy is zeroed out), why do you do
that here?

https://codereview.appspot.com/7346052/diff/1/environs/local/storage_test.go#newcode121
environs/local/storage_test.go:121: c.Check(data, DeepEquals, contents)
Mixing Assert and Check seems a bit random here, care to explain or
unify these?

https://codereview.appspot.com/7346052/

« Back to merge proposal