Don't fight gnuflag; make use of its --help feature

Don't fight gnuflag; make use of its --help feature

When gnuflag encounters --help or -h, it calls its usage func and returns
ErrHelp. To accommodate this behaviour, and get consistent output, we supply
an empty Usage function and handle ErrHelp explicitly in Main.

Because gnuflag is now used in such a way as to suppress all "automatic"
output, there is no longer any reason to report errors in its style, and
so errors reported on the command line are now prefixed with "error: " as
originally intended; as a bonus, errors are now printed *after* help, and are
therefore rather more obvious.

https://code.launchpad.net/~fwereade/juju/go-fix-cmd-help/+merge/103448

Requires: https://code.launchpad.net/~fwereade/juju/go-fix-usage/+merge/103443

(do not edit description out of merge proposal)

[fwereade, 2012-04-25 11:38:17]

Please take a look.

[rog, 2012-04-25 11:56:03]

LGTM modulo the below.

https://codereview.appspot.com/6115048/diff/1/cmd/context.go
File cmd/context.go (right):

https://codereview.appspot.com/6115048/diff/1/cmd/context.go#newcode75
cmd/context.go:75: printErr := func() { fmt.Fprintf(ctx.Stderr, "ERROR: %v\n",
err) }
this is a bit tricky. i had to look quite carefully to see how the err variable
was being used.

how about adding an "err" argument to printErr and defining it
just after printHelp? then you avoid the need to close over err, so you could
define err locally to the switch.

[fwereade, 2012-04-25 11:57:35]

Please take a look.

[fwereade, 2012-04-25 13:44:07]

Please take a look.

[fwereade, 2012-04-25 13:44:23]

https://codereview.appspot.com/6115048/diff/1/cmd/context.go
File cmd/context.go (right):

https://codereview.appspot.com/6115048/diff/1/cmd/context.go#newcode75
cmd/context.go:75: printErr := func() { fmt.Fprintf(ctx.Stderr, "ERROR: %v\n",
err) }
On 2012/04/25 11:56:03, rog wrote:
> this is a bit tricky. i had to look quite carefully to see how the err
variable
> was being used.
> 
> how about adding an "err" argument to printErr and defining it
> just after printHelp? then you avoid the need to close over err, so you could
> define err locally to the switch.

Sounds good. Done.

[niemeyer, 2012-04-25 19:29:01]

I'm -1 on this CL. Feels like not only there's no benefit, but it is actually
more code and less readable too.

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

https://codereview.appspot.com/6115048/diff/8001/cmd/command.go#newcode29
cmd/command.go:29: // printHelp writes i's usage information and description to
output, along with
Following the suggestion from the pre-req branch, what about:

func (i *Info) Help(f *gnuflag.FlagSet) []byte

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

https://codereview.appspot.com/6115048/diff/8001/cmd/context.go#newcode73
cmd/context.go:73: printHelp := func() { c.Info().printHelp(ctx.Stderr, f) }
The new code here feels significantly worse than what is on the left-hand side
of that diff.

[fwereade, 2012-04-26 07:48:50]

Please take a look.

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

https://codereview.appspot.com/6115048/diff/8001/cmd/command.go#newcode29
cmd/command.go:29: // printHelp writes i's usage information and description to
output, along with
On 2012/04/25 19:29:01, niemeyer wrote:
> Following the suggestion from the pre-req branch, what about:
> 
> func (i *Info) Help(f *gnuflag.FlagSet) []byte

Done.

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

https://codereview.appspot.com/6115048/diff/8001/cmd/context.go#newcode73
cmd/context.go:73: printHelp := func() { c.Info().printHelp(ctx.Stderr, f) }
On 2012/04/25 19:29:01, niemeyer wrote:
> The new code here feels significantly worse than what is on the left-hand side
> of that diff.

So, it turns out that setting Usage is unnecessary; and on balance, the original
structure is better; but I think we should still handle ErrHelp. How's this?

[niemeyer, 2012-04-26 13:13:59]

Thanks, LGTM. Just a couple of minors to be considered please:

https://codereview.appspot.com/6115048/diff/10001/cmd/command.go
File cmd/command.go (right):

https://codereview.appspot.com/6115048/diff/10001/cmd/command.go#newcode26
cmd/command.go:26: // help renders i's content, along with documentation for any
It feels like this is a nice bit for the public API of the package, but I'm
happy to wait until we need it.

https://codereview.appspot.com/6115048/diff/10001/cmd/context.go
File cmd/context.go (right):

https://codereview.appspot.com/6115048/diff/10001/cmd/context.go#newcode78
cmd/context.go:78: fmt.Fprintf(ctx.Stderr, "ERROR: %v\n", err)
That's much better, thanks. These two lines can be simplified as:

fmt.Fprintf(ctx.Stderr, "error: %v\n%s", err, help)

https://codereview.appspot.com/6115048/diff/10001/cmd/context.go#newcode84
cmd/context.go:84: fmt.Fprintf(ctx.Stderr, "ERROR: %v\n", err)
I'd prefer to have that as s/ERROR/error/

[fwereade, 2012-04-26 15:53:33]

*** Submitted:

Don't fight gnuflag; make use of its --help feature

When gnuflag encounters --help or -h, it calls its usage func and returns
ErrHelp. To accommodate this behaviour, and get consistent output, we supply
an empty Usage function and handle ErrHelp explicitly in Main.

Because gnuflag is now used in such a way as to suppress all "automatic"
output, there is no longer any reason to report errors in its style, and
so errors reported on the command line are now prefixed with "error: " as
originally intended; as a bonus, errors are now printed *after* help, and are
therefore rather more obvious.

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

https://codereview.appspot.com/6115048/diff/10001/cmd/command.go
File cmd/command.go (right):

https://codereview.appspot.com/6115048/diff/10001/cmd/command.go#newcode26
cmd/command.go:26: // help renders i's content, along with documentation for any
On 2012/04/26 13:13:59, niemeyer wrote:
> It feels like this is a nice bit for the public API of the package, but I'm
> happy to wait until we need it.

Coming as part of a followup anyway :).

https://codereview.appspot.com/6115048/diff/10001/cmd/context.go
File cmd/context.go (right):

https://codereview.appspot.com/6115048/diff/10001/cmd/context.go#newcode84
cmd/context.go:84: fmt.Fprintf(ctx.Stderr, "ERROR: %v\n", err)
On 2012/04/26 13:13:59, niemeyer wrote:
> I'd prefer to have that as s/ERROR/error/

Done, along with the reordering of help/error discussed on IRC.