Code review comment for lp:~fwereade/juju-core/errors-cleanup

On 2013/06/18 23:33:17, thumper wrote:
> https://codereview.appspot.com/9830049/diff/1/charm/repo.go
> File charm/repo.go (right):

> https://codereview.appspot.com/9830049/diff/1/charm/repo.go#newcode288
> charm/repo.go:288: return errors.RawNotFoundf("charm %q not found in
repository
> %q", curl, repoPath)
> On 2013/06/12 13:51:05, rog wrote:
> > NotFoundf("charm %q in repository %q", curl, repoPath) ?

> What is the difference between NotFoundf and RawNotFoundf?

> https://codereview.appspot.com/9830049/diff/1/charm/repo_test.go
> File charm/repo_test.go (right):

https://codereview.appspot.com/9830049/diff/1/charm/repo_test.go#newcode188
> charm/repo_test.go:188: c.Assert(errors.IsNotFoundError(err), Equals,
true)
> Nice. Although how about a matcher?

> c.Assert(err, IsNotFoundError)
> or
> c.Assert(err, errors.IsNotFound)

https://codereview.appspot.com/9830049/diff/1/environs/imagemetadata/simplestreams.go
> File environs/imagemetadata/simplestreams.go (right):

https://codereview.appspot.com/9830049/diff/1/environs/imagemetadata/simplestreams.go#newcode248
> environs/imagemetadata/simplestreams.go:248: return nil, dataURL,
> errors.RawNotFoundf("invalid URL %q", dataURL)
> Is there a good reason why we have two functions? RawNotFoundf and
NotFoundf?
> Why have two same but different functions/types?

> https://codereview.appspot.com/9830049/diff/1/errors/errors.go
> File errors/errors.go (right):

https://codereview.appspot.com/9830049/diff/1/errors/errors.go#newcode11
> errors/errors.go:11: type NotFoundError struct {
> On 2013/06/12 13:51:05, rog wrote:
> > s/NotFoundError/NotFound/ ?
> > (as discussed)
> >
> > and ditto for Unauthorized and the Is* functions.

> +1 as almost always namespaced in the caller code with the package.
> errors.IsNotFound
> seems nicer to me than
> errors.IsNotFoundError

> Hopefully the package name makes it obvious.

> (Message brought to you by the department of redundancy department)

https://codereview.appspot.com/9830049/diff/1/errors/errors.go#newcode35
> errors/errors.go:35: // "foo bar".
> The name of the method and be behaviour seem to be at odds to me.

LGTM - it is better than what was there before, lets not depend on
perfection

https://codereview.appspot.com/9830049/