Code review comment for lp:~axwalk/juju-core/lp1186969-add-jujud-tools-help

On 2013/07/24 08:50:11, rog wrote:
> This looks reasonable, but I'm not convinced by it living in jujud.
The jujud
> command is really an implementation detail and there's no particular
reason that
> it will be installed on a user's machine, which is probably where
charms are
> being written. Importing jujuc into juju grows the binary by about 3%,
but I
> don't think that's a big issue.

Is it possible to have a jujud on the target system that is out of sync
with the juju client, though? The other reason that I put it in jujud
was to avoid misleading help. But I guess that's being a bit pedantic,
and probably not going to be an issue. I'll move it.

> I'd also like to see a test.

Fair enough, I'll add one.

> https://codereview.appspot.com/11758043/diff/1/cmd/jujud/helptool.go
> File cmd/jujud/helptool.go (right):

https://codereview.appspot.com/11758043/diff/1/cmd/jujud/helptool.go#newcode20
> cmd/jujud/helptool.go:20: func (_ dummyHookContext) UnitName() string
{
> s/_ //

> and below

Done.

https://codereview.appspot.com/11758043/diff/1/cmd/jujud/helptool.go#newcode79
> cmd/jujud/helptool.go:79: cmds := make([]cmd.Command, len(names))
> How about:
> cmds := make([]cmd.Command, 0, len(names))

> then append to it inside the next loop rather than assigning
> to a known index? Then you won't need the "if cmds[i] != nil"
> condition inside the print loop.

Actually that's just a brainfart; it should be make([]cmd.Command,
len(names)).
The condition will need to remain: it's there because NewCommand might
fail
in the first loop.

https://codereview.appspot.com/11758043/diff/1/cmd/jujud/helptool.go#newcode89
> cmd/jujud/helptool.go:89: const lineFormat = "%-*s %s\n"
> I don't think there's a benefit from separating the format from the
Fprintf here
> (it conceals it from govet)

Done.

https://codereview.appspot.com/11758043/diff/1/cmd/jujud/helptool.go#newcode90
> cmd/jujud/helptool.go:90: const outputFormat = "%s"
> d (I'm quite surprised the compiler didn't complain about this being
unused
> actually)

Oops - removed.

https://codereview.appspot.com/11758043/