Code review comment for lp://staging/~jimmiebtlr/juju-core/add_machines_tag

Revision history for this message
William Reade (fwereade) wrote :

LGTM with typo fix. Bonus points for idying up the other var/method
names, but not mandatory -- just sort out "cxt" and I'll approve.

(fwiw you can fix the description with `lbox propose -edit`)

https://codereview.appspot.com/98610044/diff/110001/cmd/juju/addmachine_test.go
File cmd/juju/addmachine_test.go (right):

https://codereview.appspot.com/98610044/diff/110001/cmd/juju/addmachine_test.go#newcode33
cmd/juju/addmachine_test.go:33: cxt, err := runAddMachine(c)
s/cxt/ctx/ throughout.

Or, better, s/cxt/context/ -- those few saved bytes/keystrokes really
aren't worth the mental load, especially for someone who's not already
familiar with that abbreviation.

(it took me a while to understand this myself, because ctx is such a
"natural" -- ie familiar -- contraction for me, but really: good
long(ish) variable names help everybody)

https://codereview.appspot.com/98610044/diff/110001/cmd/juju/addmachine_test.go#newcode87
cmd/juju/addmachine_test.go:87: s._assertAddContainer(c,
strconv.Itoa(i), machine, ctype)
this "_assert" is weird. Don't suppose anyone knows why we did that?

https://codereview.appspot.com/98610044/

« Back to merge proposal