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

Proposed by Dave Cheney
Status: Merged
Approved by: Gustavo Niemeyer
Approved revision: 196
Merged at revision: 202
Proposed branch: lp:~dave-cheney/pyjuju/go-state-machine-string
Merge into: lp:pyjuju/go
Diff against target: 24 lines (+6/-1)
1 file modified
state/machine.go (+6/-1)
To merge this branch: bzr merge lp:~dave-cheney/pyjuju/go-state-machine-string
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+108077@code.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/

To post a comment you must log in.
Revision history for this message
William Reade (fwereade) wrote :
Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

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 :

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/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'state/machine.go'
2--- state/machine.go 2012-05-30 01:11:22 +0000
3+++ state/machine.go 2012-06-05 22:57:21 +0000
4@@ -35,7 +35,7 @@
5 func (m *Machine) WaitAgentAlive(timeout time.Duration) error {
6 err := presence.WaitAlive(m.st.zk, m.zkAgentPath(), timeout)
7 if err != nil {
8- return fmt.Errorf("state: waiting for agent of machine %d: %v", m.Id(), err)
9+ return fmt.Errorf("state: waiting for agent of machine %s: %v", m, err)
10 }
11 return nil
12 }
13@@ -75,6 +75,11 @@
14 return err
15 }
16
17+// String returns a unique description of this machine
18+func (m *Machine) String() string {
19+ return strconv.Itoa(m.Id())
20+}
21+
22 // zkKey returns the ZooKeeper key of the machine.
23 func (m *Machine) zkKey() string {
24 return m.key

Subscribers

People subscribed via source and target branches

to all changes: