This branch refactors the Command interface.

This branch refactors the Command interface.

Firstly it separates the setting of the flags from the parsing and the command
initialisation.

Some command test helper functions also added to juju-core/testing

https://code.launchpad.net/~thumper/juju-core/command-set-flags/+merge/149183

(do not edit description out of merge proposal)

[thumper2, 2013-02-19 01:04:34]

Please take a look.

[dave_cheney.net, 2013-02-19 01:30:26]

LGTM, modulo small stylistic comments.

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

https://codereview.appspot.com/7338048/diff/1/cmd/cmd.go#newcode25
cmd/cmd.go:25: // Add command specific flags to the flag set.
The commenting style is more along the lines of

// SetFlags adds command specific flags to the flag set.

https://codereview.appspot.com/7338048/diff/1/cmd/cmd.go#newcode100
cmd/cmd.go:100: // the testing module too.
// ParseArgs encapsulates ...

https://codereview.appspot.com/7338048/diff/1/cmd/cmd.go#newcode106
cmd/cmd.go:106: // come prior to the subcommand name.
Oh, this is easier, see second form: http://golang.org/ref/spec#Type_assertions

_, allowIntersperse := c.(*SuperCommand)

https://codereview.appspot.com/7338048/diff/1/cmd/cmd.go#newcode114
cmd/cmd.go:114: if err := f.Parse(allowIntersperse, args); err != nil {
return f.Parse(...)

https://codereview.appspot.com/7338048/diff/1/cmd/cmd.go#newcode127
cmd/cmd.go:127: if err := ParseArgs(c, f, args); err != nil {
suggestion: Go style avoids nested conditionals, this could also be written as

err := ParseArgs(c, f, args)
if err == gnuflag.ErrHelp { ... }
if err != nil { ... }

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

https://codereview.appspot.com/7338048/diff/1/cmd/juju/cmd_test.go#newcode11
cmd/juju/cmd_test.go:11: juju_testing "launchpad.net/juju-core/juju/testing"
no underscores please, we usually rename the juju-core/testing import

https://codereview.appspot.com/7338048/diff/1/cmd/juju/cmd_test.go#newcode12
cmd/juju/cmd_test.go:12: "launchpad.net/juju-core/testing"
import coretesting "launchpad.net/juju-core/testing"

https://codereview.appspot.com/7338048/diff/1/cmd/juju/cmd_test.go#newcode16
cmd/juju/cmd_test.go:16: juju_testing.JujuConnSuite
see above

[dimitern, 2013-02-19 08:25:00]

LGTM, modulo the below comments.

Very nice otherwise!

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

https://codereview.appspot.com/7338048/diff/1/cmd/cmd.go#newcode108
cmd/cmd.go:108: switch c.(type) {
_, allowIntersperse := c.(*SuperCommand)

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

https://codereview.appspot.com/7338048/diff/1/cmd/juju/cmd_test.go#newcode11
cmd/juju/cmd_test.go:11: juju_testing "launchpad.net/juju-core/juju/testing"
On 2013/02/19 01:30:26, dfc wrote:
> no underscores please, we usually rename the juju-core/testing import
+1

either use coretesting "..." for juju-core/testing OR stdtesting "testing".

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

https://codereview.appspot.com/7338048/diff/1/cmd/juju/upgradejuju_test.go#ne...
cmd/juju/upgradejuju_test.go:8: jujutesting
"launchpad.net/juju-core/juju/testing"
Please, use coretesting instead, it's the usual way to import both stdtesting
and juju-core/testing

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

https://codereview.appspot.com/7338048/diff/1/cmd/supercommand.go#newcode96
cmd/supercommand.go:96: func (c *SuperCommand) SetFlags(f *gnuflag.FlagSet) {
Comment here please.
// SetFlags ...

https://codereview.appspot.com/7338048/diff/1/testing/cmd.go
File testing/cmd.go (right):

https://codereview.appspot.com/7338048/diff/1/testing/cmd.go#newcode11
testing/cmd.go:11: func NewFlagSet() *gnuflag.FlagSet {
Comment here please.

https://codereview.appspot.com/7338048/diff/1/testing/cmd.go#newcode17
testing/cmd.go:17: func InitCommand(c cmd.Command, args []string) error {
ditto

https://codereview.appspot.com/7338048/diff/1/testing/cmd.go#newcode26
testing/cmd.go:26: func RunCommand(c *C, com cmd.Command, args []string) error {
ditto

https://codereview.appspot.com/7338048/diff/1/testing/cmd.go#newcode33
testing/cmd.go:33: // testInit checks that a command initialises correctly
s/testInit/TestInit/

https://codereview.appspot.com/7338048/diff/1/worker/uniter/jujuc/unit-get_te...
File worker/uniter/jujuc/unit-get_test.go (right):

https://codereview.appspot.com/7338048/diff/1/worker/uniter/jujuc/unit-get_te...
worker/uniter/jujuc/unit-get_test.go:30: func (s *UnitGetSuite) createCommand(c
*C) cmd.Command {
Nice!

[thumper2, 2013-02-19 21:43:37]

Pushing up the changes.

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

https://codereview.appspot.com/7338048/diff/1/cmd/cmd.go#newcode25
cmd/cmd.go:25: // Add command specific flags to the flag set.
On 2013/02/19 01:30:26, dfc wrote:
> The commenting style is more along the lines of
> 
> // SetFlags adds command specific flags to the flag set.

Done.

https://codereview.appspot.com/7338048/diff/1/cmd/cmd.go#newcode100
cmd/cmd.go:100: // the testing module too.
On 2013/02/19 01:30:26, dfc wrote:
> // ParseArgs encapsulates ...

Done.

https://codereview.appspot.com/7338048/diff/1/cmd/cmd.go#newcode106
cmd/cmd.go:106: // come prior to the subcommand name.
On 2013/02/19 01:30:26, dfc wrote:
> Oh, this is easier, see second form:
http://golang.org/ref/spec#Type_assertions
> 
> _, allowIntersperse := c.(*SuperCommand)

For some reason I thought it was only interfaces in the right, but now I see
that I was mistaken.

Done.

https://codereview.appspot.com/7338048/diff/1/cmd/cmd.go#newcode108
cmd/cmd.go:108: switch c.(type) {
On 2013/02/19 08:25:00, dimitern wrote:
> _, allowIntersperse := c.(*SuperCommand)

Done.

https://codereview.appspot.com/7338048/diff/1/cmd/cmd.go#newcode114
cmd/cmd.go:114: if err := f.Parse(allowIntersperse, args); err != nil {
On 2013/02/19 01:30:26, dfc wrote:
> return f.Parse(...)

Done.

https://codereview.appspot.com/7338048/diff/1/cmd/cmd.go#newcode127
cmd/cmd.go:127: if err := ParseArgs(c, f, args); err != nil {
On 2013/02/19 01:30:26, dfc wrote:
> suggestion: Go style avoids nested conditionals, this could also be written as
> 
> err := ParseArgs(c, f, args)
> if err == gnuflag.ErrHelp { ... }
> if err != nil { ... }
> 

It was while doing this fix that I realised I had broken the --help command for
subcommands in supercommands.  Broken in the sense that it would return a
slightly mangled help as it was prefixed by "error: ", and a non-zero return
code.

This has been tweaked.

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

https://codereview.appspot.com/7338048/diff/1/cmd/juju/cmd_test.go#newcode11
cmd/juju/cmd_test.go:11: juju_testing "launchpad.net/juju-core/juju/testing"
On 2013/02/19 08:25:00, dimitern wrote:
> On 2013/02/19 01:30:26, dfc wrote:
> > no underscores please, we usually rename the juju-core/testing import
> +1
> 
> either use coretesting "..." for juju-core/testing OR stdtesting "testing".

went with coretesting

https://codereview.appspot.com/7338048/diff/1/cmd/juju/cmd_test.go#newcode11
cmd/juju/cmd_test.go:11: juju_testing "launchpad.net/juju-core/juju/testing"
On 2013/02/19 01:30:26, dfc wrote:
> no underscores please, we usually rename the juju-core/testing import

Done.

https://codereview.appspot.com/7338048/diff/1/cmd/juju/cmd_test.go#newcode12
cmd/juju/cmd_test.go:12: "launchpad.net/juju-core/testing"
On 2013/02/19 01:30:26, dfc wrote:
> import coretesting "launchpad.net/juju-core/testing"

Done.

https://codereview.appspot.com/7338048/diff/1/cmd/juju/cmd_test.go#newcode16
cmd/juju/cmd_test.go:16: juju_testing.JujuConnSuite
On 2013/02/19 01:30:26, dfc wrote:
> see above

Done.

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

https://codereview.appspot.com/7338048/diff/1/cmd/juju/upgradejuju_test.go#ne...
cmd/juju/upgradejuju_test.go:8: jujutesting
"launchpad.net/juju-core/juju/testing"
On 2013/02/19 08:25:00, dimitern wrote:
> Please, use coretesting instead, it's the usual way to import both stdtesting
> and juju-core/testing

Done.

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

https://codereview.appspot.com/7338048/diff/1/cmd/supercommand.go#newcode96
cmd/supercommand.go:96: func (c *SuperCommand) SetFlags(f *gnuflag.FlagSet) {
On 2013/02/19 08:25:00, dimitern wrote:
> Comment here please.
> // SetFlags ...

Done.

https://codereview.appspot.com/7338048/diff/1/testing/cmd.go
File testing/cmd.go (right):

https://codereview.appspot.com/7338048/diff/1/testing/cmd.go#newcode11
testing/cmd.go:11: func NewFlagSet() *gnuflag.FlagSet {
On 2013/02/19 08:25:00, dimitern wrote:
> Comment here please.

Done.

https://codereview.appspot.com/7338048/diff/1/testing/cmd.go#newcode17
testing/cmd.go:17: func InitCommand(c cmd.Command, args []string) error {
On 2013/02/19 08:25:00, dimitern wrote:
> ditto

Done.

https://codereview.appspot.com/7338048/diff/1/testing/cmd.go#newcode26
testing/cmd.go:26: func RunCommand(c *C, com cmd.Command, args []string) error {
On 2013/02/19 08:25:00, dimitern wrote:
> ditto

Done.

https://codereview.appspot.com/7338048/diff/1/testing/cmd.go#newcode33
testing/cmd.go:33: // testInit checks that a command initialises correctly
On 2013/02/19 08:25:00, dimitern wrote:
> s/testInit/TestInit/

Done.

[thumper2, 2013-02-19 21:53:16]

Please take a look.

[thumper2, 2013-02-19 22:13:50]

*** Submitted:

This branch refactors the Command interface.

Firstly it separates the setting of the flags from the parsing and the command
initialisation.

Some command test helper functions also added to juju-core/testing

R=dfc, dimitern
CC=
https://codereview.appspot.com/7338048