Rearrange Parse/ParsePositional responsibilities

Rearrange Parse/ParsePositional responsibilities

...in order to support forthcoming extraction of SuperCommand's
command-selection capability. Also:

* renamed ParsePositional to Consume, which IMO fits better
* moved CheckEmpty into embeddable FlagCommand type, which implies no
positional arguments

https://code.launchpad.net/~fwereade/juju/go-recurse-parse-directly/+merge/103064

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

(do not edit description out of merge proposal)

[fwereade, 2012-04-23 09:23:46]

Please take a look.

[niemeyer, 2012-04-24 00:11:39]

I think this is almost ready to go except for a few suggestions. I'd like to
just understand some of the details to make sure we're in the same page.
Questions follow.

https://codereview.appspot.com/6107048/diff/1/cmd/command.go
File cmd/command.go (right):

https://codereview.appspot.com/6107048/diff/1/cmd/command.go#newcode49
cmd/command.go:49: // unhandled args which will be used in a recursive Parse
call.
I don't quite follow the direction this is going. If it is *returning* unhandled
arguments, it can't be a *recursive* call. Who's recursing, and why this
interface would care about it?

https://codereview.appspot.com/6107048/diff/1/cmd/command.go#newcode98
cmd/command.go:98: leftover, err := c.Consume(f.Args())
This looks nice.

https://codereview.appspot.com/6107048/diff/1/cmd/command.go#newcode103
cmd/command.go:103: return Parse(c, leftover)
But this is weird. Having Parse(c) being called twice for the same command with
a subset of the arguments is quite strange. I thought it could be just a step
towards splitting up the logic of the supercommand, but the fact it's documented
above makes it look like this is here for staying. What's the plan around this?

https://codereview.appspot.com/6107048/diff/1/cmd/command.go#newcode109
cmd/command.go:109: type FlagCommand struct{}
I suggest organizing it like this:

func ConsumeZeroArgs(args []string) ([]string, error) {
	if len(args) != 0 {
		return fmt.Errorf("unrecognised args: %s", args)
	}
	return nil, nil
)

type ZeroArgsConsumer struct{}

func (ZeroArgsConsumer) Consume(args []string) ([]string, error) {
	return ConsumeZeroArgs(args)
}

https://codereview.appspot.com/6107048/diff/1/cmd/context_test.go
File cmd/context_test.go (right):

https://codereview.appspot.com/6107048/diff/1/cmd/context_test.go#newcode24
cmd/context_test.go:24: *cmd.FlagCommand
s/*//; it's a pointer to no data.. we may as well not have a pointer.

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

https://codereview.appspot.com/6107048/diff/1/cmd/juju/bootstrap.go#newcode12
cmd/juju/bootstrap.go:12: *cmd.FlagCommand
Ditto.

https://codereview.appspot.com/6107048/diff/1/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/6107048/diff/1/cmd/jujud/agent.go#newcode14
cmd/jujud/agent.go:14: *cmd.FlagCommand
No need to have this here.

https://codereview.appspot.com/6107048/diff/1/cmd/jujud/agent.go#newcode50
cmd/jujud/agent.go:50: return c.FlagCommand.Consume(args)
return ConsumeZeroArgs(args)

https://codereview.appspot.com/6107048/diff/1/cmd/jujud/initzk.go
File cmd/jujud/initzk.go (right):

https://codereview.appspot.com/6107048/diff/1/cmd/jujud/initzk.go#newcode11
cmd/jujud/initzk.go:11: *cmd.FlagCommand
Also not necessary.

https://codereview.appspot.com/6107048/diff/1/cmd/jujud/initzk.go#newcode46
cmd/jujud/initzk.go:46: return c.FlagCommand.Consume(args)
return ConsumeZeroArgs(args)

https://codereview.appspot.com/6107048/diff/1/cmd/supercommand.go
File cmd/supercommand.go (right):

https://codereview.appspot.com/6107048/diff/1/cmd/supercommand.go#newcode99
cmd/supercommand.go:99: // args to allow c to be reparsed as the subcommand.
The changes below are a great improvement, but the fact this is being "reparsed"
is still sad. I hope we can fix this soon.

[fwereade, 2012-04-24 08:41:25]

[this may all become moot in the context of the discussion in play on
https://codereview.appspot.com/6100050/ -- maybe read this for a better
understanding of the forces in play there, but focus on that one first]

In the first iteration of this code, we (I think) agreed that the slight
weirdness of the SuperCommand reparsing *itself* was an acceptable price for a
solution which allowed for `juju bootstrap -v` rather than forcing `juju -v
bootstrap` (but (a) still allowed that style, for python compatibility and (b)
allowed for a one-way knowledge flow (commands don't have any concept of
parents).

When first attempting the change in https://codereview.appspot.com/6100050/ I
was baffled for an embarrassingly long time; I had split the types, and
SuperCommand's (then) ParsePositional method was calling Parse again (with
itself, as always) and wait, whoa, why don't the logging flags work?

Of course the logging flags didn't work because the SuperCommand.Parse was just
calling Parse on the embedded SuperCommand, not the outer type, and so wasn't
picking up the logging flags. D'oh. So, I made the recursive Parse (which was
always there) direct... and here we are.

I do have a suggestion; first, some introduction, so you can stop me right away
if my assumptions aren't shared.

The general idea of Parse being recursive is not IMO a bad one: there's no
fundamental difference between parsing a top-level command and parsing a
subcommand. However, a Parse function that repeatedly parses the same Command
*is* weird, and AIUI thats the heart of your concern.

Perspective: bearing in mind the constraint described in the first paragraph, we
actually do have to reparse the same Command, so that shared flags can behave
correctly. I think the important thing is not to remove this precise mechanism,
but to remove the *assumption* of this mechanism in Parse.

So, suggestion: ParsePositional/Consume will (1) take a new name to reflect new
responsibilities still better; and (2) will return both the leftover args *and*
the subcommand that needs to be parsed. This intuitively fits in with what the
supercommand is doing -- it selects a command and delegates further arg-handling
to that command -- and lets us continue to use the existing mechanism. That is,
the "subcommand that needs to be parsed" is still the supercommand, but the
Parse function doesn't know that and thereby becomes saner.

Um, I suspect I could have said this all much more tersely...

[fwereade, 2012-04-24 08:53:21]

On 2012/04/24 08:41:25, fwereade wrote:
> So, suggestion: ParsePositional/Consume will (1) take a new name to reflect
new
> responsibilities still better

... you know, I think the best name is actually "Parse":

type Command interface {
    func Parse(args []string) (subcmd Command, subargs []string, err error)
    ...

...and then the (bare) Parse would look something like:

func Parse(c Command, args []string) (err error) {
    for {
        f := newFlagSet(c, ioutil.Discard)
        if err = f.Parse(c.Info().Intersperse, args); err != nil {
            return err
        }
        c, args, err = c.Parse(f.Args())
        if err != nil {
            return err
        }
        if c == nil {
            return nil
        }
    }
}

...but I can imagine rational beings hating the fact that "Parse" has 3 distinct
meanings in the same piece or code. Thoughts?

[PS: discussion at https://codereview.appspot.com/6100050/ *may* be relevant for
context]

[fwereade, 2012-04-24 09:20:53]

On 2012/04/24 08:53:21, fwereade wrote:
> ...but I can imagine rational beings hating the fact that "Parse" has 3
distinct
> meanings in the same piece or code. Thoughts?

OK, I've changed my mind, Parse is a bad name. Thinking.

[fwereade, 2012-04-24 10:31:26]

Hmm.

  func Reduce(args []string) (Command, []string, error)
  ...
  c, args, err = c.Reduce(f.Args())

...perhaps?

[fwereade, 2012-04-24 10:46:25]

(awaiting discussion re reparsing; other points addressed)

https://codereview.appspot.com/6107048/diff/1/cmd/command.go
File cmd/command.go (right):

https://codereview.appspot.com/6107048/diff/1/cmd/command.go#newcode109
cmd/command.go:109: type FlagCommand struct{}
On 2012/04/24 00:11:39, niemeyer wrote:
> I suggest organizing it like this:
> 
> func ConsumeZeroArgs(args []string) ([]string, error) {
> 	if len(args) != 0 {
> 		return fmt.Errorf("unrecognised args: %s", args)
> 	}
> 	return nil, nil
> )
> 
> type ZeroArgsConsumer struct{}
> 
> func (ZeroArgsConsumer) Consume(args []string) ([]string, error) {
> 	return ConsumeZeroArgs(args)
> }

Nice. Done.

https://codereview.appspot.com/6107048/diff/1/cmd/context_test.go
File cmd/context_test.go (right):

https://codereview.appspot.com/6107048/diff/1/cmd/context_test.go#newcode24
cmd/context_test.go:24: *cmd.FlagCommand
On 2012/04/24 00:11:39, niemeyer wrote:
> s/*//; it's a pointer to no data.. we may as well not have a pointer. 

Done.

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

https://codereview.appspot.com/6107048/diff/1/cmd/juju/bootstrap.go#newcode12
cmd/juju/bootstrap.go:12: *cmd.FlagCommand
On 2012/04/24 00:11:39, niemeyer wrote:
> Ditto.

Done.

https://codereview.appspot.com/6107048/diff/1/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/6107048/diff/1/cmd/jujud/agent.go#newcode14
cmd/jujud/agent.go:14: *cmd.FlagCommand
On 2012/04/24 00:11:39, niemeyer wrote:
> No need to have this here.

Done.

https://codereview.appspot.com/6107048/diff/1/cmd/jujud/agent.go#newcode50
cmd/jujud/agent.go:50: return c.FlagCommand.Consume(args)
On 2012/04/24 00:11:39, niemeyer wrote:
> return ConsumeZeroArgs(args)

Done.

https://codereview.appspot.com/6107048/diff/1/cmd/jujud/initzk.go
File cmd/jujud/initzk.go (right):

https://codereview.appspot.com/6107048/diff/1/cmd/jujud/initzk.go#newcode11
cmd/jujud/initzk.go:11: *cmd.FlagCommand
On 2012/04/24 00:11:39, niemeyer wrote:
> Also not necessary.

Done.

https://codereview.appspot.com/6107048/diff/1/cmd/jujud/initzk.go#newcode46
cmd/jujud/initzk.go:46: return c.FlagCommand.Consume(args)
On 2012/04/24 00:11:39, niemeyer wrote:
> return ConsumeZeroArgs(args)

Done.