Make SuperCommand convenient to use with forthcoming cmd/server

Make SuperCommand convenient to use with forthcoming cmd/server

prereq: lp:~fwereade/juju/go-add-cmd-context

* expose new field SetsLog; if false, log-related options are not handled
  and ctx.InitLog will not be called.
* expose new field Purpose; allows SCs to provide slightly better output
  when desired.
* the command's full name is now used in the error returned when an
  unknown subcommand is recognised.

https://code.launchpad.net/~fwereade/juju/go-tweak-supercommand/+merge/96136

Requires: https://code.launchpad.net/~fwereade/juju/go-add-cmd-context/+merge/96131

(do not edit description out of merge proposal)

[fwereade, 2012-03-06 14:50:57]

Please take a look.

[fwereade, 2012-03-17 01:11:20]

Please take a look.

[rog, 2012-03-22 11:13:42]

LGTM

https://codereview.appspot.com/5756054/diff/9001/cmd/supercommand.go
File cmd/supercommand.go (right):

https://codereview.appspot.com/5756054/diff/9001/cmd/supercommand.go#newcode23
cmd/supercommand.go:23: Purpose string
it would be nice to have some comments describing these fields. for instance,
how is Purpose different from Doc. what does SetsLog imply? etc.

[niemeyer, 2012-03-28 02:47:18]

Related to the summary:

"""
* expose new field SetsLog; if false, log-related options are not handled and
ctx.InitLog will not be called.
"""

Why do we have ctx.InitLog then? Isn't that precisely so that logging may be
handled differently in the server?

https://codereview.appspot.com/5756054/diff/9001/cmd/supercommand.go
File cmd/supercommand.go (right):

https://codereview.appspot.com/5756054/diff/9001/cmd/supercommand.go#newcode88
cmd/supercommand.go:88: if c.SetsLog {
It feels unnecessary to do this here. Logging flags should always be made
available, otherwise anything done with the log package will be trashed. Even if
at the moment a given command happen to not log anything, I'd still prefer to
have that available for all commands than having this special casing here.

https://codereview.appspot.com/5756054/diff/9001/cmd/supercommand.go#newcode96
cmd/supercommand.go:96: f.BoolVar(&c.Debug, "d", c.Debug, "if set, log debugging
messages")
On a note unrelated to this branch, this flag looks bogus. We don't need such a
first class shorthand for debugging juju.

[fwereade, 2012-04-19 11:55:16]

https://codereview.appspot.com/5756054/diff/9001/cmd/supercommand.go
File cmd/supercommand.go (right):

https://codereview.appspot.com/5756054/diff/9001/cmd/supercommand.go#newcode88
cmd/supercommand.go:88: if c.SetsLog {
On 2012/03/28 02:47:18, niemeyer wrote:
> It feels unnecessary to do this here. Logging flags should always be made
> available, otherwise anything done with the log package will be trashed. Even
if
> at the moment a given command happen to not log anything, I'd still prefer to
> have that available for all commands than having this special casing here.

To recap:

(1) jujuc commands perform the "meat" of their work within a host process
(jujud).
(2) therefore, the bits that are most important to log are happening within the
host process.
(3) we don't want to allow log setup in a jujuc command to break the logging in
the host process.
(4) you rejected a proposal in which I added a layer of indirection in the log
package, which would have allowed us to have separate logs for jujuc commands
(the cost of doing so is in passing that proposed log object around to the
things which need it, and I understand your reservations there).
(5) in the course of that discussion we ended up agreeing that logging flags are
useless for jujuc commands.
(6) SuperCommand handles subcommand selection very nicely, and IMO it's sensible
to reuse that functionality when the host process is interpreting a command line
in order to figure out which command to run.
(7) (the present day again) you're saying that SuperCommand should always make
the logging flags available.

-----

The only way forward that I can see -- that doesn't go directly against your
stated preferences -- is to split SuperCommand up so that there's a type that
handles subcommand selection, and another that handles the logging flags (and
probably embeds the selector type); juju and jujud use the logging type, and
jujuc commands are selected with the plain selector type.

My gut says that a couple of "if"s in SuperCommand are a better solution than
splitting the type this way, but I can see arguments in both directions. Please
clarify your position a little... I'm not sure whether the proposed split is a
good response to your concerns, or whether there's a third way that I haven't
seen.

https://codereview.appspot.com/5756054/diff/9001/cmd/supercommand.go#newcode96
cmd/supercommand.go:96: f.BoolVar(&c.Debug, "d", c.Debug, "if set, log debugging
messages")
On 2012/03/28 02:47:18, niemeyer wrote:
> On a note unrelated to this branch, this flag looks bogus. We don't need such
a
> first class shorthand for debugging juju.

Happy to drop it, I'll do another branch.