pyjuju

Merge lp://staging/~dave-cheney/pyjuju/go-state-machine-string into lp://staging/pyjuju/go

go-state-machine-string
Merge into go

Proposed by Dave Cheney on 2012-05-30

Status:	Merged
Approved by:	Gustavo Niemeyer on 2012-06-06
Approved revision:	196
Merged at revision:	202
Proposed branch:	lp://staging/~dave-cheney/pyjuju/go-state-machine-string
Merge into:	lp://staging/pyjuju/go
Diff against target:	24 lines (+6/-1) 1 file modified state/machine.go (+6/-1)
To merge this branch:	bzr merge lp://staging/~dave-cheney/pyjuju/go-state-machine-string
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Juju Engineering		2012-05-30	Pending
Review via email: mp+108077@code.staging.launchpad.net

Description of the change

state: add Machine.String()

This proposal allows state.Machine to fulfil the Stringer
interface so it is capable of printing itself in a way that
is not dependant on it's internal represntation.

https://codereview.appspot.com/6257072/

Revision history for this message

William Reade (fwereade) wrote on 2012-06-01:

LGTM

https://codereview.appspot.com/6257072/

Revision history for this message

Gustavo Niemeyer (niemeyer) wrote on 2012-06-05:

LGTM assuming you're happy with the following suggestions.

https://codereview.appspot.com/6257072/diff/6001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/6257072/diff/6001/state/machine.go#newcode38
state/machine.go:38: return fmt.Errorf("state: waiting for agent of %s:
%v", m, err)
s/state://
s/of /of machine/

https://codereview.appspot.com/6257072/diff/6001/state/machine.go#newcode80
state/machine.go:80: return fmt.Sprintf("machine %d", m.Id())
I think this should be something like:

return strconv.Itoa(m.Id())

We already have other String methods returning identifiers, and they do
not qualify the value type. We either have to change all, or follow with
the convention.

Form another angle, it's not usual to qualify in that way, as rather
than an identifier this is an excerpt which enforces a way to construct
the sentence. Imagine writing a sentence like "can't connect machines %s
and %s", for example.

https://codereview.appspot.com/6257072/

Revision history for this message

Gustavo Niemeyer (niemeyer) wrote on 2012-06-06:

You mentioned changes to the tests, but there's nothing in this branch
about that. What's here still LGTM.

https://codereview.appspot.com/6257072/