state: Rework of the errors in machine.go.

state: Rework of the errors in machine.go.

Next step in the reworking process of the state errors,
this time the file machine.go.

https://code.launchpad.net/~themue/juju-core/go-state-errors-machine/+merge/110513

(do not edit description out of merge proposal)

[TheMue, 2012-06-15 12:46:36]

Please take a look.

[fwereade, 2012-06-15 20:42:56]

LGTM, but fix inconsistency below please

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

https://codereview.appspot.com/6304084/diff/1/state/machine.go#newcode34
state/machine.go:34: return fmt.Errorf("waiting for agent of machine %s: %v", m,
err)
Inconsistent with "machine %d"s below.

[niemeyer, 2012-06-16 01:48:45]

LGTM with the following addressed.

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

https://codereview.appspot.com/6304084/diff/1/state/machine.go#newcode34
state/machine.go:34: return fmt.Errorf("waiting for agent of machine %s: %v", m,
err)
On 2012/06/15 20:42:56, fwereade wrote:
> Inconsistent with "machine %d"s below.

Agreed. Let's have all of them as m and %s, please.

https://codereview.appspot.com/6304084/diff/1/state/machine.go#newcode54
state/machine.go:54: // missing key is fine
Can you please replace this comment by

// TODO Return NoInstanceIdError (also when it exists as "")

If one asks for the InstanceId, it's *not* fine to not give it back silently, as
this is extremely error prone. We need a public NoInstanceIdError error type
that can be verified by callers (and that stringifies nicely), but that prevents
them from mistakingly assume they got an instance id if err == nil.

https://codereview.appspot.com/6304084/diff/1/state/machine.go#newcode60
state/machine.go:60: return "", fmt.Errorf("invalid internal machine key type:
%T", v)
("invalid internal machine id type %T for machine %s", v, m)

https://codereview.appspot.com/6304084/diff/1/state/machine.go#newcode88
state/machine.go:88: defer errorContextf(&err, "can't set instance id of machine
%d", m.Id())
("can't set instance id of machine %s to %q", m, id)

[TheMue, 2012-06-18 17:15:17]

*** Submitted:

state: Rework of the errors in machine.go.

Next step in the reworking process of the state errors,
this time the file machine.go.

R=fwereade, niemeyer
CC=
https://codereview.appspot.com/6304084

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

https://codereview.appspot.com/6304084/diff/1/state/machine.go#newcode34
state/machine.go:34: return fmt.Errorf("waiting for agent of machine %s: %v", m,
err)
On 2012/06/16 01:48:45, niemeyer wrote:
> On 2012/06/15 20:42:56, fwereade wrote:
> > Inconsistent with "machine %d"s below.
> 
> Agreed. Let's have all of them as m and %s, please.

Done.

https://codereview.appspot.com/6304084/diff/1/state/machine.go#newcode54
state/machine.go:54: // missing key is fine
On 2012/06/16 01:48:45, niemeyer wrote:
> Can you please replace this comment by
> 
> // TODO Return NoInstanceIdError (also when it exists as "")
> 
> If one asks for the InstanceId, it's *not* fine to not give it back silently,
as
> this is extremely error prone. We need a public NoInstanceIdError error type
> that can be verified by callers (and that stringifies nicely), but that
prevents
> them from mistakingly assume they got an instance id if err == nil.

Done.

https://codereview.appspot.com/6304084/diff/1/state/machine.go#newcode60
state/machine.go:60: return "", fmt.Errorf("invalid internal machine key type:
%T", v)
On 2012/06/16 01:48:45, niemeyer wrote:
> ("invalid internal machine id type %T for machine %s", v, m)

Done.

https://codereview.appspot.com/6304084/diff/1/state/machine.go#newcode88
state/machine.go:88: defer errorContextf(&err, "can't set instance id of machine
%d", m.Id())
On 2012/06/16 01:48:45, niemeyer wrote:
> ("can't set instance id of machine %s to %q", m, id)

Done.