Code review comment for lp:~rogpeppe/pyjuju/go-state-units-under-service

Very nice change. LGTM, assuming you're happy with the adaptations
below. Otherwise, please reply accordingly and repropose.

Please note you'll have to propose+submit this to the new juju-core
project, since the old trunk is now gone (sorry!). You can do this by
creating a new branch from the new project as you'd usually do, merging
from this work onto it, and then just proposing as usual.

https://codereview.appspot.com/6247066/diff/7001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/6247066/diff/7001/state/machine.go#newcode88
state/machine.go:88: // keyId returns the id corresponding to the given
machine or unit id.
s/keyId/keySequence/ or s/keyId/keySeq/

This is not an identifier. "unit-0000001" is the identifier, and we use
the term "key" there, after much debate, precisely to avoid the
confusion with the idea of an "id" which is public. We can easily avoid
the confusion still by calling the sequence number what it really is (it
comes from the idea of sequence from ZooKeeper).

As a special case, we do call the *machine* sequence number an
identifier, because that's how we handle it across the board
(machine.Id(), etc).

https://codereview.appspot.com/6247066/diff/7001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6247066/diff/7001/state/service.go#newcode13
state/service.go:13: pathPkg "path"
stdpath

https://codereview.appspot.com/6247066/diff/7001/state/service.go#newcode84
state/service.go:84: kprefix = kprefix[:strings.LastIndex(kprefix,
"-")+1]
There's no reason to go over the trouble here. It's already assuming
knowledge about how makeUnitKey works (who said it could skip 'til the
last dash like that?) so it may as well be a lot more clear and say
"/units/unit-" + s.key + "-".

https://codereview.appspot.com/6247066/diff/7001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6247066/diff/7001/state/unit.go#newcode77
state/unit.go:77: func mkUnitKey(serviceKey string, unitId int) string {
On 2012/06/04 07:29:19, fwereade wrote:
> What does "mk" signify?

I assume "make", but I'd also prefer the word itself rather than saving
two chars. Even more when we have serviceKeyForUnitKey below. :-)

https://codereview.appspot.com/6247066/diff/7001/state/unit.go#newcode86
state/unit.go:86: return "", fmt.Errorf("invalid unit key %q (1) %s",
unitKey, debug.Callers(0, 10))
On 2012/06/04 07:29:19, fwereade wrote:
> Is this the proposed implementation or an oversight?

Yeah, looks like an oversight. We need something closer to the error
message of makeUnitKey.

https://codereview.appspot.com/6247066/diff/7001/state/unit.go#newcode518
state/unit.go:518: func parseUnitName(name string) (serviceName string,
serviceId int, err error) {
s/serviceId/serviceSeq/

https://codereview.appspot.com/6247066/diff/7001/state/unit.go#newcode523
state/unit.go:523: id, err := strconv.ParseInt(parts[1], 10, 0)
s/id/seq/

https://codereview.appspot.com/6247066/diff/7001/state/unit.go#newcode527
state/unit.go:527: return parts[0], int(id), nil
s/id/seq/

https://codereview.appspot.com/6247066/