Add cmd.Context type to enable output capture and environment control

Add cmd.Context type to enable output capture and environment control

In order to run a Command inside another process, the host process needs to
know the client's working directory (for relative file paths), and it needs
separate output streams (so that it goes where it's intended).

The only impact on existing Commands is that their Run method now expects a
*cmd.Context; the only command that's materially affected is SuperCommand,
which becomes slightly simpler (because logging setup is now the
responsibility of the Context; SuperCommand just parses the args and hands
them over).

FlagSets are now always constructed with ContinueOnError, and with
configurable output; this unifies error handling and usage printing (there's
now only one code path for each), and allows us to correctly print usage for
commands that don't have any sensible options (eg open-port). As a
side-effect, it's much simpler to test command-line parsing for arbitrary
commands, because errors no longer cause os.Exit to be called. For clarity's
sake, pre-existing tests have been left untouched; expect a followup branch
that tidies up main and main_test in juju and jujud.

PrintUsage and NewFlagSet are now private (in addition to expecting an
io.Writer for output), because they weren't used externally in the first
place, and in practice were just dirtying up the interface.

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:41:58]

Please take a look.

[fwereade, 2012-03-09 09:37:09]

Please take a look.

[fwereade, 2012-03-13 09:31:39]

Please take a look.

[fwereade, 2012-03-13 09:32:11]

Please take a look.

[niemeyer, 2012-03-16 23:23:54]

That one looks pretty nice, thanks William.

https://codereview.appspot.com/5758050/diff/8001/cmd/command.go
File cmd/command.go (right):

https://codereview.appspot.com/5758050/diff/8001/cmd/command.go#newcode67
cmd/command.go:67: // options directs PrintDefaults on c's FlagSet to a
temporary buffer and
Can't this be done a lot more cleanly with something like:

// hasOptions returns whether f has any options defined.
func hasOptions(f *gnuflag.FlagSet) (opts bool) {
    f.VisitAll(func(f *gnuflag.Flag) { opts = true })
    return
}

It's also not necessary to generate a second *FlagSet within hasOptions or
printUsage. printUsage is called from a context that has a flag set available.
It may take an extra parameter with it.

https://codereview.appspot.com/5758050/diff/8001/cmd/command.go#newcode116
cmd/command.go:116: func Main(c Command, args []string) {
This function can go away. Whoever calls it can call the real thing just as
easily and more clearly.

https://codereview.appspot.com/5758050/diff/8001/cmd/context.go
File cmd/context.go (right):

https://codereview.appspot.com/5758050/diff/8001/cmd/context.go#newcode13
cmd/context.go:13: // to allow use of the cmd package within a host process.
The cmd package is _always_ used within a host process. I know what you intend
to say here, but this needs to be made more clear.

https://codereview.appspot.com/5758050/diff/8001/cmd/context.go#newcode43
cmd/context.go:43: func (ctx *Context) InitLog(verbose bool, debug bool, logfile
string) (err error) {
Looks very nice.

https://codereview.appspot.com/5758050/diff/8001/cmd/context.go#newcode71
cmd/context.go:71: if err := c.Run(ctx); err != nil {
The overall organization of Context within this file looks really nice, but the
method Main here is an exception that would be nice to clean up.

This method creates a dependency and knowledge loop where Context.Main takes
Command as an argument which calls Command.Run which takes Context as an
argument.

This should be a function, to kill that relationship loop:

    func Main(c Command, ctx *Context, args []string) int

https://codereview.appspot.com/5758050/diff/8001/cmd/context_test.go
File cmd/context_test.go (right):

https://codereview.appspot.com/5758050/diff/8001/cmd/context_test.go#newcode15
cmd/context_test.go:15: func getContext(c *C) *cmd.Context {
s/getContext/dummyContext/ (or similar.. get tends to mean something else)

https://codereview.appspot.com/5758050/diff/8001/cmd/context_test.go#newcode17
cmd/context_test.go:17: c.MkDir(), bytes.NewBuffer([]byte{}),
bytes.NewBuffer([]byte{})}
return &cmd.Context{c.MkDir(), &bytes.Buffer{}, &bytes.Buffer{}}

https://codereview.appspot.com/5758050/diff/8001/cmd/context_test.go#newcode55
cmd/context_test.go:55: var (
The block syntax here is introducing noise rather than helping, IMO.

https://codereview.appspot.com/5758050/diff/8001/cmd/context_test.go#newcode65
cmd/context_test.go:65: minUsage  = `usage: cmd-name
For example, compare that:

    `
            minUsage  = `usage: cmd-name
    `
    )

To this:

    var minUsage = "usage: cmd-name\n"

https://codereview.appspot.com/5758050/diff/8001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/5758050/diff/8001/cmd/juju/bootstrap.go#newcode39
cmd/juju/bootstrap.go:39: func (c *BootstrapCommand) Run(ctx *cmd.Context) error
{
Please use _ as a variable name for all these parameters. It's a nice way to say
that it's not in use.

[fwereade, 2012-03-17 01:01:23]

https://codereview.appspot.com/5758050/diff/8001/cmd/command.go
File cmd/command.go (right):

https://codereview.appspot.com/5758050/diff/8001/cmd/command.go#newcode67
cmd/command.go:67: // options directs PrintDefaults on c's FlagSet to a
temporary buffer and
On 2012/03/16 23:23:54, niemeyer wrote:
> Can't this be done a lot more cleanly with something like:
> 
> // hasOptions returns whether f has any options defined.
> func hasOptions(f *gnuflag.FlagSet) (opts bool) {
>     f.VisitAll(func(f *gnuflag.Flag) { opts = true })
>     return
> }
> 
> It's also not necessary to generate a second *FlagSet within hasOptions or
> printUsage. printUsage is called from a context that has a flag set available.
> It may take an extra parameter with it.

I think it is necessary to create the FlagSet in printUsage, but I think that's
OK; it all turned out quite nice in the end IMO. Calling it Done :).

https://codereview.appspot.com/5758050/diff/8001/cmd/command.go#newcode116
cmd/command.go:116: func Main(c Command, args []string) {
On 2012/03/16 23:23:54, niemeyer wrote:
> This function can go away. Whoever calls it can call the real thing just as
> easily and more clearly.

Done.

https://codereview.appspot.com/5758050/diff/8001/cmd/context.go
File cmd/context.go (right):

https://codereview.appspot.com/5758050/diff/8001/cmd/context.go#newcode13
cmd/context.go:13: // to allow use of the cmd package within a host process.
On 2012/03/16 23:23:54, niemeyer wrote:
> The cmd package is _always_ used within a host process. I know what you intend
> to say here, but this needs to be made more clear.

Done. Maybe :).

https://codereview.appspot.com/5758050/diff/8001/cmd/context.go#newcode43
cmd/context.go:43: func (ctx *Context) InitLog(verbose bool, debug bool, logfile
string) (err error) {
On 2012/03/16 23:23:54, niemeyer wrote:
> Looks very nice.
Thanks :)

https://codereview.appspot.com/5758050/diff/8001/cmd/context.go#newcode71
cmd/context.go:71: if err := c.Run(ctx); err != nil {
On 2012/03/16 23:23:54, niemeyer wrote:
> The overall organization of Context within this file looks really nice, but
the
> method Main here is an exception that would be nice to clean up.
> 
> This method creates a dependency and knowledge loop where Context.Main takes
> Command as an argument which calls Command.Run which takes Context as an
> argument.
> 
> This should be a function, to kill that relationship loop:
> 
>     func Main(c Command, ctx *Context, args []string) int

Done.

https://codereview.appspot.com/5758050/diff/8001/cmd/context_test.go
File cmd/context_test.go (right):

https://codereview.appspot.com/5758050/diff/8001/cmd/context_test.go#newcode15
cmd/context_test.go:15: func getContext(c *C) *cmd.Context {
On 2012/03/16 23:23:54, niemeyer wrote:
> s/getContext/dummyContext/ (or similar.. get tends to mean something else)

Done.

https://codereview.appspot.com/5758050/diff/8001/cmd/context_test.go#newcode17
cmd/context_test.go:17: c.MkDir(), bytes.NewBuffer([]byte{}),
bytes.NewBuffer([]byte{})}
On 2012/03/16 23:23:54, niemeyer wrote:
> return &cmd.Context{c.MkDir(), &bytes.Buffer{}, &bytes.Buffer{}}

Done.

https://codereview.appspot.com/5758050/diff/8001/cmd/context_test.go#newcode55
cmd/context_test.go:55: var (
On 2012/03/16 23:23:54, niemeyer wrote:
> The block syntax here is introducing noise rather than helping, IMO.
Agreed.

https://codereview.appspot.com/5758050/diff/8001/cmd/context_test.go#newcode65
cmd/context_test.go:65: minUsage  = `usage: cmd-name
On 2012/03/16 23:23:54, niemeyer wrote:
> For example, compare that:
> 
>     `
>             minUsage  = `usage: cmd-name
>     `
>     )
> 
> To this:
> 
>     var minUsage = "usage: cmd-name\n"

Done.

https://codereview.appspot.com/5758050/diff/8001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/5758050/diff/8001/cmd/juju/bootstrap.go#newcode39
cmd/juju/bootstrap.go:39: func (c *BootstrapCommand) Run(ctx *cmd.Context) error
{
On 2012/03/16 23:23:54, niemeyer wrote:
> Please use _ as a variable name for all these parameters. It's a nice way to
say
> that it's not in use.

Done.

[fwereade, 2012-03-17 01:04:10]

Please take a look.

[rog, 2012-03-22 11:09:08]

LGTM

https://codereview.appspot.com/5758050/diff/13001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/5758050/diff/13001/cmd/juju/bootstrap.go#newco...
cmd/juju/bootstrap.go:39: func (c *BootstrapCommand) Run(_ *cmd.Context) error {
you don't even need the _ here, but gustavo might think it looks better that
way.

[fwereade, 2012-04-19 13:16:39]

Please take a look.

[fwereade, 2012-04-23 08:42:23]

Please take a look.

[fwereade, 2012-04-23 09:00:41]

Please take a look.

[niemeyer, 2012-04-23 23:39:14]

LGTM. Thanks!

https://codereview.appspot.com/5758050/diff/8001/cmd/context.go
File cmd/context.go (right):

https://codereview.appspot.com/5758050/diff/8001/cmd/context.go#newcode13
cmd/context.go:13: // to allow use of the cmd package within a host process.
On 2012/03/17 01:01:23, fwereade wrote:
> On 2012/03/16 23:23:54, niemeyer wrote:
> > The cmd package is _always_ used within a host process. I know what you
intend
> > to say here, but this needs to be made more clear.
> 
> Done. Maybe :).

Yeah, new doc is great. Thanks!

[fwereade, 2012-04-24 07:30:31]

*** Submitted:

Add cmd.Context type to enable output capture and environment control

In order to run a Command inside another process, the host process needs to
know the client's working directory (for relative file paths), and it needs
separate output streams (so that it goes where it's intended).

The only impact on existing Commands is that their Run method now expects a
*cmd.Context; the only command that's materially affected is SuperCommand,
which becomes slightly simpler (because logging setup is now the
responsibility of the Context; SuperCommand just parses the args and hands
them over).

FlagSets are now always constructed with ContinueOnError, and with
configurable output; this unifies error handling and usage printing (there's
now only one code path for each), and allows us to correctly print usage for
commands that don't have any sensible options (eg open-port). As a
side-effect, it's much simpler to test command-line parsing for arbitrary
commands, because errors no longer cause os.Exit to be called. For clarity's
sake, pre-existing tests have been left untouched; expect a followup branch
that tidies up main and main_test in juju and jujud.

PrintUsage and NewFlagSet are now private (in addition to expecting an
io.Writer for output), because they weren't used externally in the first
place, and in practice were just dirtying up the interface.

R=niemeyer, rog
CC=
https://codereview.appspot.com/5758050