state: getter methods return NotFoundError

state: getter methods return NotFoundError

Due to the in-progress lifecycle changes, client code
must be able to distinguish not found errors.

https://code.launchpad.net/~niemeyer/juju-core/resource-not-found/+merge/125917

(do not edit description out of merge proposal)

[niemeyer, 2012-09-24 03:50:54]

Please take a look.

[niemeyer, 2012-09-24 04:08:27]

Please take a look.

[dave_cheney.net, 2012-09-24 05:20:35]

LGTM. Thank you, this is a significant improvement.

https://codereview.appspot.com/6549056/diff/2002/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6549056/diff/2002/state/state.go#newcode53
state/state.go:53: 
comment please.

https://codereview.appspot.com/6549056/diff/2002/state/state.go#newcode384
state/state.go:384: func (s *State) Relation(endpoints ...RelationEndpoint) (r
*Relation, err error) {
suggestion: drop the named return values as we're not using defer ErrorContextf
anymore in this method.

[TheMue, 2012-09-24 09:37:11]

LGTM, only a small comment.

https://codereview.appspot.com/6549056/diff/2002/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6549056/diff/2002/state/state.go#newcode55
state/state.go:55: return &NotFoundError{format, args}
If no later access to the args is planned the error message could already be
generated here. But this forces to always use this constructor. In my personal
APIs I like something like

type NotFoundError struct {
    What string
    Id   string
}

Error() creates the message, IsXyzError() bool like here and if the user needs
it he can make a type assert and access informations like the Id/Key. But this
only for public errors.

[rog, 2012-09-24 14:12:03]

looks reasonable, but one thought below.

https://codereview.appspot.com/6549056/diff/2002/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6549056/diff/2002/state/state.go#newcode46
state/state.go:46: format string
i'm not sure that i see the benefit in storing the format and args here. are we
really concerned that the work involved in formatting the error message is going
to slow us down?

it's also potentially dangerous - if we pass in a mutable value whose String
value can change, the error may change value after creation.

[niemeyer, 2012-09-24 14:38:25]

*** Submitted:

state: getter methods return NotFoundError

Due to the in-progress lifecycle changes, client code
must be able to distinguish not found errors.

R=dfc, TheMue, rog
CC=
https://codereview.appspot.com/6549056

https://codereview.appspot.com/6549056/diff/2002/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6549056/diff/2002/state/state.go#newcode46
state/state.go:46: format string
On 2012/09/24 14:12:04, rog wrote:
> i'm not sure that i see the benefit in storing the format and args here. are
we
> really concerned that the work involved in formatting the error message is
going
> to slow us down?

I certainly don't care enough to argue. I'll change it.

https://codereview.appspot.com/6549056/diff/2002/state/state.go#newcode53
state/state.go:53: 
On 2012/09/24 05:20:35, dfc wrote:
> comment please.

I honestly think it's not worth it in this case. Reading the body of the
function is more clear and takes less time than reading any comments we add
here, and since it's a private function it won't show up in the docs.

https://codereview.appspot.com/6549056/diff/2002/state/state.go#newcode55
state/state.go:55: return &NotFoundError{format, args}
On 2012/09/24 09:37:11, TheMue wrote:
> If no later access to the args is planned the error message could already be

Changed so NotFoundError has a dumb msg argument.

https://codereview.appspot.com/6549056/diff/2002/state/state.go#newcode384
state/state.go:384: func (s *State) Relation(endpoints ...RelationEndpoint) (r
*Relation, err error) {
On 2012/09/24 05:20:35, dfc wrote:
> suggestion: drop the named return values as we're not using defer
ErrorContextf
> anymore in this method.

Done.