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
Overall LGTM, modulo some trivial suggestions.
https:/ /codereview. appspot. com/7346052/ diff/1/ environs/ local/storage. go local/storage. go (right):
File environs/
https:/ /codereview. appspot. com/7346052/ diff/1/ environs/ local/storage. go#newcode11 local/storage. go:11:
environs/
d
https:/ /codereview. appspot. com/7346052/ diff/1/ environs/ local/storage. go#newcode14 local/storage. go:14:
environs/
var _ environ.Storage = (*storage)(nil) ?
https:/ /codereview. appspot. com/7346052/ diff/1/ environs/ local/storage. go#newcode30 local/storage. go:30: // exist, it should return a r/NotFoundError - or even use roger's NotFoundf() ?
environs/
*NotFoundError.
s/*NotFoundErro
https:/ /codereview. appspot. com/7346052/ diff/1/ environs/ local/storage. go#newcode68 local/storage. go:68: if string(body) == "" {
environs/
len(body) == 0 ?
https:/ /codereview. appspot. com/7346052/ diff/1/ environs/ local/storage. go#newcode76 local/storage. go:76: // URL returns a URL that can be used to
environs/
access the given storage file.
s/a URL/an URL
https:/ /codereview. appspot. com/7346052/ diff/1/ environs/ local/storage. go#newcode82 local/storage. go:82: // The length must give the total length
environs/
of the file.
s/must give/must be set to/
https:/ /codereview. appspot. com/7346052/ diff/1/ environs/ local/storage. go#newcode98 local/storage. go:98: return errors. New(resp. Status)
environs/
Include the StatusCode in the error text?
https:/ /codereview. appspot. com/7346052/ diff/1/ environs/ local/storage. go#newcode120 local/storage. go:120: return errors. New(resp. Status)
environs/
as above
https:/ /codereview. appspot. com/7346052/ diff/1/ environs/ local/storage_ test.go local/storage_ test.go (right):
File environs/
https:/ /codereview. appspot. com/7346052/ diff/1/ environs/ local/storage_ test.go# newcode70 local/storage_ test.go: 70: c.Assert(err, IsNil)
environs/
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 local/storage_ test.go: 83: for a := attempt.Start(); a.Next(); {
environs/
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 local/storage_ test.go: 121: c.Check(data, DeepEquals, contents)
environs/
Mixing Assert and Check seems a bit random here, care to explain or
unify these?
https:/ /codereview. appspot. com/7346052/