state: machine.InstanceId error proposal

state: machine.InstanceId error proposal

machine.InstanceId returns NoInstanceIdError when 
there is no instance id set in the machine *ConfigNode.

https://code.launchpad.net/~dave-cheney/juju-core/go-state-machine-id-returns-error-alternative/+merge/111208

(do not edit description out of merge proposal)

[dfc, 2012-06-20 12:30:43]

Please take a look.

[niemeyer, 2012-06-20 20:07:09]

https://codereview.appspot.com/6297105/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/6297105/diff/1/state/machine.go#newcode14
state/machine.go:14: var NoInstanceIdError = fmt.Errorf("no instance id set")
This might be better as an error type:

type NoInstanceIdError struct {
    MachineId int
}

func (e *NoInstanceIdError) Error() string {
    return fmt.Errorf("instance id for machine %d is not set", e.MachineId)
}

https://codereview.appspot.com/6297105/diff/1/state/machine.go#newcode58
state/machine.go:58: return "", false, nil
Thanks for taking care of this, and I apologize as I probably wasn't clear in
the previous conversation.

The return above is still not alright, as it's returning a nil error even though
it's failing to satisfy the request for an instance id, which is the only
purpose of this method. We also don't need a bool value in the middle, because
the lack of an id can easily be expressed as the error itself.

There are only two cases: instance id is available or it's not. If it is
available, return id, nil. If it's not, return "", &NoInstanceIdError{m.Id()}.

[dfc, 2012-06-20 22:12:57]

On 2012/06/20 20:07:09, niemeyer wrote:
> https://codereview.appspot.com/6297105/diff/1/state/machine.go
> File state/machine.go (right):
> 
> https://codereview.appspot.com/6297105/diff/1/state/machine.go#newcode14
> state/machine.go:14: var NoInstanceIdError = fmt.Errorf("no instance id set")
> This might be better as an error type:
> 
> type NoInstanceIdError struct {
>     MachineId int
> }
> 
> func (e *NoInstanceIdError) Error() string {
>     return fmt.Errorf("instance id for machine %d is not set", e.MachineId)
> }
> 
> https://codereview.appspot.com/6297105/diff/1/state/machine.go#newcode58
> state/machine.go:58: return "", false, nil
> Thanks for taking care of this, and I apologize as I probably wasn't clear in
> the previous conversation.
> 
> The return above is still not alright, as it's returning a nil error even
though
> it's failing to satisfy the request for an instance id, which is the only
> purpose of this method. We also don't need a bool value in the middle, because
> the lack of an id can easily be expressed as the error itself.
> 
> There are only two cases: instance id is available or it's not. If it is
> available, return id, nil. If it's not, return "", &NoInstanceIdError{m.Id()}.

No, this was not your fault. 

The reason I proposed this form was when doing a more traditional two return
value version there ended up being a lot of duplication when using this method,
eg

id, err := m.InstanceId()
if err == state.NoInstanceIdError { 
   // could be ok, if we were just checking to see if this machine was started
}
if err != nil {
   // handle unexpected error
}
// id is valid

I'll propose this other version for comparison.

[dfc, 2012-06-21 03:12:39]

Please take a look.

[dfc, 2012-06-21 03:26:25]

Please take a look.

[niemeyer, 2012-06-21 16:48:56]

https://codereview.appspot.com/6297105/diff/10001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/6297105/diff/10001/state/machine.go#newcode14
state/machine.go:14: type NoInstanceIdError int
I think we really want the suggested struct here. Having an int makes it look
like the integer is some sort of error code.

[dfc, 2012-06-22 01:56:06]

Please take a look.

https://codereview.appspot.com/6297105/diff/10001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/6297105/diff/10001/state/machine.go#newcode14
state/machine.go:14: type NoInstanceIdError int
On 2012/06/21 16:48:56, niemeyer wrote:
> I think we really want the suggested struct here. Having an int makes it look
> like the integer is some sort of error code.

Done.

[niemeyer, 2012-06-22 20:24:08]

Thanks, and I'm sorry that this simple change is being more painful that it
should. There are still a few details there:

https://codereview.appspot.com/6297105/diff/6008/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6297105/diff/6008/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:216: if _, ok := err.(state.NoInstanceIdError); !ok {
Please see the other comments regarding the use of pointers.

https://codereview.appspot.com/6297105/diff/6008/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:235: if _, ok := err.(state.NoInstanceIdError); !ok {
Please see the other comments regarding the use of pointers.

https://codereview.appspot.com/6297105/diff/6008/cmd/jujud/provisioning_test.go
File cmd/jujud/provisioning_test.go (right):

https://codereview.appspot.com/6297105/diff/6008/cmd/jujud/provisioning_test....
cmd/jujud/provisioning_test.go:135: if _, ok := err.(state.NoInstanceIdError);
!ok {
Please see the other comments regarding the use of pointers.

https://codereview.appspot.com/6297105/diff/6008/cmd/jujud/provisioning_test....
cmd/jujud/provisioning_test.go:151: _, ok := err.(state.NoInstanceIdError)
Please see the other comments regarding the use of pointers.

https://codereview.appspot.com/6297105/diff/6008/cmd/jujud/provisioning_test....
cmd/jujud/provisioning_test.go:156: if ok {
if _, ok := ...; ok {
    return
}
c.Check(err, IsNil)

https://codereview.appspot.com/6297105/diff/6008/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/6297105/diff/6008/state/machine.go#newcode64
state/machine.go:64: return "", NoInstanceIdError{m.Id()}
s/No/&No/

For some reason error values are provided as pointers pretty much always. While
there's no reason for this specific type to be a pointer, it's useful to follow
that convention as call sites can generally assume that it is a pointer when
doing err.(*NoInstanceIdError) assertions. Otherwise, we'd have to look at the
*implementation* to see what to do in each case.

https://codereview.appspot.com/6297105/diff/6008/state/machine.go#newcode68
state/machine.go:68: return "", NoInstanceIdError{m.Id()}
Ditto.

[dfc, 2012-06-25 01:20:10]

Please take a look.

https://codereview.appspot.com/6297105/diff/6008/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6297105/diff/6008/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:216: if _, ok := err.(state.NoInstanceIdError); !ok {
On 2012/06/22 20:24:08, niemeyer wrote:
> Please see the other comments regarding the use of pointers.

Done.

https://codereview.appspot.com/6297105/diff/6008/cmd/jujud/provisioning_test.go
File cmd/jujud/provisioning_test.go (right):

https://codereview.appspot.com/6297105/diff/6008/cmd/jujud/provisioning_test....
cmd/jujud/provisioning_test.go:135: if _, ok := err.(state.NoInstanceIdError);
!ok {
On 2012/06/22 20:24:08, niemeyer wrote:
> Please see the other comments regarding the use of pointers.

Done.

https://codereview.appspot.com/6297105/diff/6008/cmd/jujud/provisioning_test....
cmd/jujud/provisioning_test.go:151: _, ok := err.(state.NoInstanceIdError)
On 2012/06/22 20:24:08, niemeyer wrote:
> Please see the other comments regarding the use of pointers.

Done.

https://codereview.appspot.com/6297105/diff/6008/cmd/jujud/provisioning_test....
cmd/jujud/provisioning_test.go:156: if ok {
On 2012/06/22 20:24:08, niemeyer wrote:
> if _, ok := ...; ok {
>     return
> }
> c.Check(err, IsNil)

Done.

https://codereview.appspot.com/6297105/diff/6008/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/6297105/diff/6008/state/machine.go#newcode64
state/machine.go:64: return "", NoInstanceIdError{m.Id()}
On 2012/06/22 20:24:08, niemeyer wrote:
> s/No/&No/
> 
> For some reason error values are provided as pointers pretty much always.
While
> there's no reason for this specific type to be a pointer, it's useful to
follow
> that convention as call sites can generally assume that it is a pointer when
> doing err.(*NoInstanceIdError) assertions. Otherwise, we'd have to look at the
> *implementation* to see what to do in each case.

Done. Makes sense

https://codereview.appspot.com/6297105/diff/6008/state/machine.go#newcode68
state/machine.go:68: return "", NoInstanceIdError{m.Id()}
On 2012/06/22 20:24:08, niemeyer wrote:
> Ditto.

Done.

[dfc, 2012-06-25 01:21:34]

Thanks for your review, this should be all that is required now.

[niemeyer, 2012-06-25 04:04:25]

LGTM, thanks Dave. Just one trivial before submitting if you don't mind:

https://codereview.appspot.com/6297105/diff/22001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6297105/diff/22001/state/state_test.go#newcode233
state/state_test.go:233: c.Assert(err, FitsTypeOf, new(state.NoInstanceIdError))
&Type{} is the convention we generally stick to. This is definitely a matter of
choice, but since we've been pretty consistent I'd prefer to keep it.

[dfc, 2012-06-25 06:59:29]

*** Submitted:

state: machine.InstanceId error proposal

machine.InstanceId returns NoInstanceIdError when 
there is no instance id set in the machine *ConfigNode.

R=niemeyer
CC=
https://codereview.appspot.com/6297105