Add a composed base class for commands.

Add a composed base class for commands.

This allows overriding of how the help command works, and provides a
default implementation for normal commands.

https://code.launchpad.net/~thumper/juju-core/command-base/+merge/150483

Requires: https://code.launchpad.net/~thumper/juju-core/refactor-command-context/+merge/150253

(do not edit description out of merge proposal)

[thumper, 2013-02-28 03:08:37]

Please take a look.

[dimitern, 2013-02-28 09:10:54]

NOT LGTM until this is clarified - it seems it removes functionality which was
there and no reasons are given about it.

https://codereview.appspot.com/7375053/diff/2001/cmd/cmd.go
File cmd/cmd.go (right):

https://codereview.appspot.com/7375053/diff/2001/cmd/cmd.go#newcode42
cmd/cmd.go:42: // SetFlags does nothing in the simplest case.
// SetFlags does nothing for the base command. ?

https://codereview.appspot.com/7375053/diff/2001/cmd/cmd.go#newcode45
cmd/cmd.go:45: // Init in the simplest case makes sure there are no args.
s/in the simplest case//

https://codereview.appspot.com/7375053/diff/2001/cmd/cmd.go#newcode50
cmd/cmd.go:50: // Help by default prints out the generated help from the Info.
// Help returns the generated help using the given Info. ?

https://codereview.appspot.com/7375053/diff/2001/cmd/juju/addrelation.go
File cmd/juju/addrelation.go (left):

https://codereview.appspot.com/7375053/diff/2001/cmd/juju/addrelation.go#oldc...
cmd/juju/addrelation.go:12: EnvName   string
Why?

https://codereview.appspot.com/7375053/diff/2001/cmd/juju/addrelation.go#oldc...
cmd/juju/addrelation.go:24: func (c *AddRelationCommand) SetFlags(f
*gnuflag.FlagSet) {
Why is this removed? And the EnvName up there?

[rog, 2013-02-28 12:31:57]

i'm not sure about this change. some thoughts below.

https://codereview.appspot.com/7375053/diff/2001/cmd/cmd.go
File cmd/cmd.go (right):

https://codereview.appspot.com/7375053/diff/2001/cmd/cmd.go#newcode36
cmd/cmd.go:36: Help(i *Info, f *gnuflag.FlagSet) []byte
what's the rationale for this method?

AFAICS there's not a problem with implementing everything that we need without
this method, which isn't implemented anywhere except in CommandBase (and seems
to be the main reason that we're adding CommandBase)

https://codereview.appspot.com/7375053/diff/2001/cmd/juju/addrelation.go
File cmd/juju/addrelation.go (left):

https://codereview.appspot.com/7375053/diff/2001/cmd/juju/addrelation.go#oldc...
cmd/juju/addrelation.go:24: func (c *AddRelationCommand) SetFlags(f
*gnuflag.FlagSet) {
On 2013/02/28 09:10:55, dimitern wrote:
> Why is this removed? And the EnvName up there?

i see why it's removed (EnvCommandBase.SetFlags adds the relevant environ
flags), but i'm not sure whether this change is a net improvement overall.

i think i prefer that it's obvious what flags a given command implements even at
the expense of some repetition of (trivial) code. as things are, i can look at
the implementation of any command and see at a glance what flags and arguments
it takes. this change makes those flags and arguments one or two levels of
indirection away, and implicit.

if the code being factored out was in any way complex, i'd see more
justification for the change, but AFAICS it makes the code harder to read and
not significantly easier to maintain.

[thumper, 2013-03-01 04:37:36]

Please take a look.

[thumper, 2013-03-01 04:39:56]

On 2013/02/28 12:31:57, rog wrote:
> i'm not sure about this change. some thoughts below.
> 
> https://codereview.appspot.com/7375053/diff/2001/cmd/cmd.go
> File cmd/cmd.go (right):
> 
> https://codereview.appspot.com/7375053/diff/2001/cmd/cmd.go#newcode36
> cmd/cmd.go:36: Help(i *Info, f *gnuflag.FlagSet) []byte
> what's the rationale for this method?

Due to changes in the following branch, this isn't needed any more.

> AFAICS there's not a problem with implementing everything that we need without
> this method, which isn't implemented anywhere except in CommandBase (and seems
> to be the main reason that we're adding CommandBase)

That and to provide some basis for Don't Repeat Yourself (DRY)

Code that can avoid duplication should avoid it.

I have to disagree that repetition of code is a good thing.  It is a very bad
thing and should be avoided.

The addition of the CommandBase and the EnvCommandBase are there to avoid the
duplication of code in the same way the common object oriented code has base
classes.  Go provides us a way to do this with composition and syntactic sugar
for methods on those embedded structs, and we should use them.

[rog, 2013-03-01 10:53:03]

On 1 March 2013 04:39,  <tim.penhey@canonical.com> wrote:
> On 2013/02/28 12:31:57, rog wrote:
>>
>> i'm not sure about this change. some thoughts below.
>
>
>> https://codereview.appspot.com/7375053/diff/2001/cmd/cmd.go
>> File cmd/cmd.go (right):
>
>
>> https://codereview.appspot.com/7375053/diff/2001/cmd/cmd.go#newcode36
>> cmd/cmd.go:36: Help(i *Info, f *gnuflag.FlagSet) []byte
>> what's the rationale for this method?
>
>
> Due to changes in the following branch, this isn't needed any more.
>
>
>> AFAICS there's not a problem with implementing everything that we need
>
> without
>>
>> this method, which isn't implemented anywhere except in CommandBase
>
> (and seems
>>
>> to be the main reason that we're adding CommandBase)
>
>
> That and to provide some basis for Don't Repeat Yourself (DRY)
>
> Code that can avoid duplication should avoid it.
>
> I have to disagree that repetition of code is a good thing.  It is a
> very bad thing and should be avoided.

I think I have a somewhat more pragmatic attitude to DRY - I think
there's often a trade-off between overall simplicity and DRY,
and I tend to prefer simple code over not-so-simple code with no redundancy.
I think overall maintainability should be the principal criterion, and DRY
only helps with one aspect of that (the case where we want to
change all the things at once).

In this case we save a few lines of code overall but pay the price that
you have to look in three different places to see what flags
and arguments a command takes.

I don't think it's worth it, but if others do, please say so. I admit
that this decision is very much a matter of taste.

[dimitern, 2013-03-01 12:20:24]

> I think I have a somewhat more pragmatic attitude to DRY - I think
> there's often a trade-off between overall simplicity and DRY,
> and I tend to prefer simple code over not-so-simple code with no redundancy.
> I think overall maintainability should be the principal criterion, and DRY
> only helps with one aspect of that (the case where we want to
> change all the things at once).
> 
> In this case we save a few lines of code overall but pay the price that
> you have to look in three different places to see what flags
> and arguments a command takes.
> 
> I don't think it's worth it, but if others do, please say so. I admit
> that this decision is very much a matter of taste.

+1 on all that.

[dave_cheney.net, 2013-03-04 15:31:35]

> I don't think it's worth it, but if others do, please say so. I admit
> that this decision is very much a matter of taste.

I believe this is a worthwhile change. I am a strong +1 on this (this is a
capital plus).

[dimitern, 2013-03-07 22:36:25]

LGTM, after discussion and agreement - this is actually a good change and I like
it! Just a couple of comments.

https://codereview.appspot.com/7375053/diff/9001/cmd/juju/environmentcommand.go
File cmd/juju/environmentcommand.go (right):

https://codereview.appspot.com/7375053/diff/9001/cmd/juju/environmentcommand....
cmd/juju/environmentcommand.go:16: f.StringVar(&c.EnvName, "e", "", "juju
environment to operate in")
Integrate JUJU_ENV stuff before landing please.

https://codereview.appspot.com/7375053/diff/9001/doc/draft/commands.txt
File doc/draft/commands.txt (right):

https://codereview.appspot.com/7375053/diff/9001/doc/draft/commands.txt#newco...
doc/draft/commands.txt:16: Supercommands
Is this now base command instead?

[thumper, 2013-03-08 14:04:53]

https://codereview.appspot.com/7375053/diff/9001/cmd/juju/environmentcommand.go
File cmd/juju/environmentcommand.go (right):

https://codereview.appspot.com/7375053/diff/9001/cmd/juju/environmentcommand....
cmd/juju/environmentcommand.go:16: f.StringVar(&c.EnvName, "e", "", "juju
environment to operate in")
On 2013/03/07 22:36:25, dimitern wrote:
> Integrate JUJU_ENV stuff before landing please.

Done.

https://codereview.appspot.com/7375053/diff/9001/doc/draft/commands.txt
File doc/draft/commands.txt (right):

https://codereview.appspot.com/7375053/diff/9001/doc/draft/commands.txt#newco...
doc/draft/commands.txt:16: Supercommands
On 2013/03/07 22:36:25, dimitern wrote:
> Is this now base command instead?

No, they are independent.

[dave_cheney.net, 2013-03-08 14:12:35]

LGTM once the JUJU_ENV change is integrated.

https://codereview.appspot.com/7375053/diff/9001/cmd/juju/environmentcommand.go
File cmd/juju/environmentcommand.go (right):

https://codereview.appspot.com/7375053/diff/9001/cmd/juju/environmentcommand....
cmd/juju/environmentcommand.go:16: f.StringVar(&c.EnvName, "e", "", "juju
environment to operate in")
On 2013/03/08 14:04:53, thumper wrote:
> On 2013/03/07 22:36:25, dimitern wrote:
> > Integrate JUJU_ENV stuff before landing please.
> 
> Done.

Doesn't appear to be done. Do you need to rerun lbox propose.

[thumper, 2013-03-08 15:04:07]

Please take a look.

[thumper, 2013-03-08 15:35:46]

*** Submitted:

Add a composed base class for commands.

This allows overriding of how the help command works, and provides a
default implementation for normal commands.

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