Code review comment for lp:~fwereade/pyjuju/go-cmd-context

Revision history for this message
William Reade (fwereade) wrote :

Reviewers: mp+95602_code.launchpad.net,

Message:
Please take a look.

Description:
prereq: lp:~fwereade/juju/go-log-type

The top-level Main, Run, Parse and NewFlagSet function have all become
methods
on the new Context type, as has SuperCommand.initOutput. Main and Parse,
which
are both used by code outside this package, have been retained for
convenience's sake and act as they did before.

The Command interface has changed slightly; Run now expects a *Context,
and
ParsePositional can now return a []string to indicate that the Command
should
be Parsed again with the returned args; this allows us to move the
re-Parsing
logic to Context.Parse; even though SuperCommand is the only user, this
allows us to avoid passing a Context into ParsePositional, which feels
quite
inappropriate. (Given that every Command has to implement
ParsePositional,
it's better to add additional nil return values everywhere than to pass
an
unused Context around everywhere).

This doesn't feel like an overwhelmingly large change, but it doesn't
diff
very nicely. Most of the changes should be explained by the above;
there's
also an unrelated type rename in supercommand_test.go because, er, it
seemed
like a good idea at the time (and still doesn't feel like a bad one,
even if
it adds to the noise a bit).

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

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/5719050/

Affected files:
   M cmd/command.go
   A cmd/context.go
   A cmd/context_test.go
   M cmd/juju/bootstrap.go
   M cmd/juju/main.go
   M cmd/jujud/agent.go
   M cmd/jujud/initzk.go
   M cmd/jujud/machine.go
   M cmd/jujud/main.go
   M cmd/jujud/provisioning.go
   M cmd/jujud/unit.go
   M cmd/supercommand.go
   M cmd/supercommand_test.go
   M log/log.go
   M log/log_test.go
   M store/store_test.go

« Back to merge proposal