Moved printUsage to Info

Moved printUsage to Info

It became apparent that printUsage wasn't really using a Command, it was
just using its Info so heavily as to imply it wanted to be a method on Info;
and, having moved printUsage to Info, it became clear that options are the
sole preserve of the gnuflag.FlagSet, and should not be mentioned in
Info.Args (because to do so is to assert knowledge about the state of a
shared resource, the FlagSet, without legitimately being able to do so). So,
printUsage is now also responsible for inserting `[options]` into the usage
line, in exactly the same way as it does the section actually describing
those options (ie: it only does so if they exist).

As a side-effect, SuperCommand `[options]` are placed before `<command> ...`
in the usage line for the bare SuperCommand; this is IMO sensible because
we're displaying the options that apply whatever command you select, and
which actually *do* work in that position; ofc, when displaying help for a
particular subcommand, we put the options after the command (and before its
positional arguments). So we get:

  usage: juju [options] <command> ...

...and:

  usage: juju bootstrap [options]

...each of which accurately captures the possibilities in the appropriate
situation.

This branch also includes slightly heavier testing for SuperCommand's Info,
and small tweaks to ensure consistent output of SuperCommand documentation
as expected by those tests.

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

(do not edit description out of merge proposal)

[fwereade, 2012-04-25 10:39:27]

Please take a look.

[fwereade, 2012-04-25 10:48:11]

Please take a look.

[rog, 2012-04-25 11:38:24]

LGTM modulo doc suggestions and trivials.

https://codereview.appspot.com/6123049/diff/3001/cmd/command.go
File cmd/command.go (right):

https://codereview.appspot.com/6123049/diff/3001/cmd/command.go#newcode31
cmd/command.go:31: // its output stream should be set again after calling this
method.
I think this message assumes too much about caller behaviour.
How about something like this?

// printUsage prints usage information for i with the given
// flags. It calls f.SetOutput(ioutil.Discard).

https://codereview.appspot.com/6123049/diff/3001/cmd/context.go
File cmd/context.go (right):

https://codereview.appspot.com/6123049/diff/3001/cmd/context.go#newcode15
cmd/context.go:15: // to allow Commands to be run without using the current
process's working
s/Commands/the Command/

but this comment feels a bit like a bit like a "design" comment rather than
something that belongs on Context.

i think i'd prefer to see something that focused more
on how it's meant to be used. sorry i didn't mention this
earlier.

// Context represents the run context of a Command.
// Command implementations should interpret file names
// relative to Dir (see AbsPath below), and print output
// and errors to Stdout and Stderr respectively.

https://codereview.appspot.com/6123049/diff/3001/cmd/context.go#newcode70
cmd/context.go:70: // Main will Init and Run a Command, in the supplied Context,
and return a
this reads a little awkwardly to me. i'm not sure you need to specifically
mention that it calls Init and Run - that's part of the Command contract.

how about something like this:

// Main runs the given Command in the supplied Context
// with the given arguments, which should not include
// the command name.  It returns a code
// suitable for passing to os.Exit.

https://codereview.appspot.com/6123049/diff/3001/cmd/supercommand.go
File cmd/supercommand.go (right):

https://codereview.appspot.com/6123049/diff/3001/cmd/supercommand.go#newcode51
cmd/supercommand.go:51: cmds := make([]string, count)
i think this could be marginally simpler at no cost:
cmds := make([]string, len(c.subcmds))
if len(cmds) == 0 {
    return ""
}

(for the record make([]T, 0) does not do an allocation)

https://codereview.appspot.com/6123049/diff/3001/cmd/supercommand.go#newcode53
cmd/supercommand.go:53: for name, _ := range c.subcmds {
s/, _//

[fwereade, 2012-04-25 11:53:07]

Please take a look.

https://codereview.appspot.com/6123049/diff/3001/cmd/command.go
File cmd/command.go (right):

https://codereview.appspot.com/6123049/diff/3001/cmd/command.go#newcode31
cmd/command.go:31: // its output stream should be set again after calling this
method.
On 2012/04/25 11:38:24, rog wrote:
> I think this message assumes too much about caller behaviour.
> How about something like this?
> 
> // printUsage prints usage information for i with the given
> // flags. It calls f.SetOutput(ioutil.Discard).

Done, with a tweak to make it always call that, for sanity's sake.

https://codereview.appspot.com/6123049/diff/3001/cmd/context.go
File cmd/context.go (right):

https://codereview.appspot.com/6123049/diff/3001/cmd/context.go#newcode15
cmd/context.go:15: // to allow Commands to be run without using the current
process's working
On 2012/04/25 11:38:24, rog wrote:
> s/Commands/the Command/
> 
> but this comment feels a bit like a bit like a "design" comment rather than
> something that belongs on Context.
> 
> i think i'd prefer to see something that focused more
> on how it's meant to be used. sorry i didn't mention this
> earlier.
> 
> // Context represents the run context of a Command.
> // Command implementations should interpret file names
> // relative to Dir (see AbsPath below), and print output
> // and errors to Stdout and Stderr respectively.

Nice; done.

https://codereview.appspot.com/6123049/diff/3001/cmd/context.go#newcode70
cmd/context.go:70: // Main will Init and Run a Command, in the supplied Context,
and return a
On 2012/04/25 11:38:24, rog wrote:
> this reads a little awkwardly to me. i'm not sure you need to specifically
> mention that it calls Init and Run - that's part of the Command contract.
> 
> how about something like this:
> 
> // Main runs the given Command in the supplied Context
> // with the given arguments, which should not include
> // the command name.  It returns a code
> // suitable for passing to os.Exit.

Done.

https://codereview.appspot.com/6123049/diff/3001/cmd/supercommand.go
File cmd/supercommand.go (right):

https://codereview.appspot.com/6123049/diff/3001/cmd/supercommand.go#newcode51
cmd/supercommand.go:51: cmds := make([]string, count)
On 2012/04/25 11:38:24, rog wrote:
> i think this could be marginally simpler at no cost:
> cmds := make([]string, len(c.subcmds))
> if len(cmds) == 0 {
>     return ""
> }
> 
> (for the record make([]T, 0) does not do an allocation)

Done.

https://codereview.appspot.com/6123049/diff/3001/cmd/supercommand.go#newcode53
cmd/supercommand.go:53: for name, _ := range c.subcmds {
On 2012/04/25 11:38:24, rog wrote:
> s/, _//

Done.

[niemeyer, 2012-04-25 19:17:04]

https://codereview.appspot.com/6123049/diff/7001/cmd/command.go
File cmd/command.go (right):

https://codereview.appspot.com/6123049/diff/7001/cmd/command.go#newcode14
cmd/command.go:14: // support code, and cannot be determined by examining a
Command in isolation.
This description feels confusing.. it talks about unknown "support code", the
inability of defining the flag set looking at a Command, etc. Why does Info care
about those things?

IMO, this should look like:

// Info holds some of the usage documentation for a command.

https://codereview.appspot.com/6123049/diff/7001/cmd/command.go#newcode31
cmd/command.go:31: func (i *Info) printUsage(output io.Writer, f
*gnuflag.FlagSet) {
What about:

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

?

https://codereview.appspot.com/6123049/diff/7001/cmd/command.go#newcode47
cmd/command.go:47: f.PrintDefaults()
For now, this is a red-herring, but I'd prefer to have a mechanism to avoid the
SetOutput hackery here in the unknown future, given that this seems like a good
piece for the API.

https://codereview.appspot.com/6123049/diff/7001/cmd/supercommand.go
File cmd/supercommand.go (right):

https://codereview.appspot.com/6123049/diff/7001/cmd/supercommand.go#newcode74
cmd/supercommand.go:74: if cmds := strings.TrimSpace(c.describeCommands()); cmds
!= "" {
Why would describeCommands return something with spaces if we don't want them?
It's the only usage of our own private method.

https://codereview.appspot.com/6123049/diff/7001/cmd/supercommand_test.go
File cmd/supercommand_test.go (right):

https://codereview.appspot.com/6123049/diff/7001/cmd/supercommand_test.go#new...
cmd/supercommand_test.go:70: c.Assert(info.Doc, Equals, "commands:\n   
defenestrate defenestrate the juju")
There seems to be a good hint here that the formatting needs some love.

[fwereade, 2012-04-26 07:28:24]

Please take a look.

https://codereview.appspot.com/6123049/diff/7001/cmd/command.go
File cmd/command.go (right):

https://codereview.appspot.com/6123049/diff/7001/cmd/command.go#newcode14
cmd/command.go:14: // support code, and cannot be determined by examining a
Command in isolation.
On 2012/04/25 19:17:04, niemeyer wrote:
> This description feels confusing.. it talks about unknown "support code", the
> inability of defining the flag set looking at a Command, etc. Why does Info
care
> about those things?
> 
> IMO, this should look like:
> 
> // Info holds some of the usage documentation for a command.

Ha, yeah, that's much better. Done.

https://codereview.appspot.com/6123049/diff/7001/cmd/command.go#newcode31
cmd/command.go:31: func (i *Info) printUsage(output io.Writer, f
*gnuflag.FlagSet) {
On 2012/04/25 19:17:04, niemeyer wrote:
> What about:
> 
> func (i *Info) Usage(f *gnuflag.FlagSet) []byte
> 
> ?

Hmm, nice. Done.

https://codereview.appspot.com/6123049/diff/7001/cmd/command.go#newcode47
cmd/command.go:47: f.PrintDefaults()
On 2012/04/25 19:17:04, niemeyer wrote:
> For now, this is a red-herring, but I'd prefer to have a mechanism to avoid
the
> SetOutput hackery here in the unknown future, given that this seems like a
good
> piece for the API.

Agreed.

https://codereview.appspot.com/6123049/diff/7001/cmd/supercommand.go
File cmd/supercommand.go (right):

https://codereview.appspot.com/6123049/diff/7001/cmd/supercommand.go#newcode74
cmd/supercommand.go:74: if cmds := strings.TrimSpace(c.describeCommands()); cmds
!= "" {
On 2012/04/25 19:17:04, niemeyer wrote:
> Why would describeCommands return something with spaces if we don't want them?
> It's the only usage of our own private method.

Er, good point. Done.

https://codereview.appspot.com/6123049/diff/7001/cmd/supercommand_test.go
File cmd/supercommand_test.go (right):

https://codereview.appspot.com/6123049/diff/7001/cmd/supercommand_test.go#new...
cmd/supercommand_test.go:70: c.Assert(info.Doc, Equals, "commands:\n   
defenestrate defenestrate the juju")
On 2012/04/25 19:17:04, niemeyer wrote:
> There seems to be a good hint here that the formatting needs some love.

Done.

[niemeyer, 2012-04-26 14:16:41]

LGTM. Just a suggestion:

https://codereview.appspot.com/6123049/diff/12001/cmd/supercommand.go
File cmd/supercommand.go (right):

https://codereview.appspot.com/6123049/diff/12001/cmd/supercommand.go#newcode60
cmd/supercommand.go:60: sort.Strings(cmds)
format := "    %-" + strconv.Itoa(longest) + "s - %s"

(note the dash.. I think it'll look more clear with it)

https://codereview.appspot.com/6123049/diff/12001/cmd/supercommand.go#newcode64
cmd/supercommand.go:64: cmds[i] = fmt.Sprintf("    %s  %s", padded, purpose)
cmds[i] = fmt.Sprintf(format, name, purpose)

[rog, 2012-04-26 14:23:08]

https://codereview.appspot.com/6123049/diff/12001/cmd/supercommand.go
File cmd/supercommand.go (right):

https://codereview.appspot.com/6123049/diff/12001/cmd/supercommand.go#newcode64
cmd/supercommand.go:64: cmds[i] = fmt.Sprintf("    %s  %s", padded, purpose)
On 2012/04/26 14:16:41, niemeyer wrote:
> cmds[i] = fmt.Sprintf(format, name, purpose)

alternatively:
cmds[i] = fmt.Sprintf("     %-*s - %s", longest, padded, purpose)

[niemeyer, 2012-04-26 14:34:16]

> alternatively:
> cmds[i] = fmt.Sprintf("     %-*s - %s", longest, padded, purpose)

Hah, even nicer (s/padded/name/).

[fwereade, 2012-04-26 15:06:47]

*** Submitted:

Moved printUsage to Info

It became apparent that printUsage wasn't really using a Command, it was
just using its Info so heavily as to imply it wanted to be a method on Info;
and, having moved printUsage to Info, it became clear that options are the
sole preserve of the gnuflag.FlagSet, and should not be mentioned in
Info.Args (because to do so is to assert knowledge about the state of a
shared resource, the FlagSet, without legitimately being able to do so). So,
printUsage is now also responsible for inserting `[options]` into the usage
line, in exactly the same way as it does the section actually describing
those options (ie: it only does so if they exist).

As a side-effect, SuperCommand `[options]` are placed before `<command> ...`
in the usage line for the bare SuperCommand; this is IMO sensible because
we're displaying the options that apply whatever command you select, and
which actually *do* work in that position; ofc, when displaying help for a
particular subcommand, we put the options after the command (and before its
positional arguments). So we get:

  usage: juju [options] <command> ...

...and:

  usage: juju bootstrap [options]

...each of which accurately captures the possibilities in the appropriate
situation.

This branch also includes slightly heavier testing for SuperCommand's Info,
and small tweaks to ensure consistent output of SuperCommand documentation
as expected by those tests.

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

https://codereview.appspot.com/6123049/diff/12001/cmd/supercommand.go
File cmd/supercommand.go (right):

https://codereview.appspot.com/6123049/diff/12001/cmd/supercommand.go#newcode60
cmd/supercommand.go:60: sort.Strings(cmds)
On 2012/04/26 14:16:41, niemeyer wrote:
> format := "    %-" + strconv.Itoa(longest) + "s - %s"
> 
> (note the dash.. I think it'll look more clear with it)

Done.

https://codereview.appspot.com/6123049/diff/12001/cmd/supercommand.go#newcode64
cmd/supercommand.go:64: cmds[i] = fmt.Sprintf("    %s  %s", padded, purpose)
On 2012/04/26 14:16:41, niemeyer wrote:
> cmds[i] = fmt.Sprintf(format, name, purpose)

Done.