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

Revision history for this message
Roger Peppe (rogpeppe) 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.

I'd also like to see a test.

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

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.

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)

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)

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

« Back to merge proposal