Code review comment for lp:~axwalk/juju-core/envcmd-inversion

On 2014/05/12 07:53:07, axw wrote:
> On 2014/05/12 07:24:18, fwereade wrote:
> > This LGTM as far as it goes (modulo the need for testing -- I'd like
it to be
> > impossible to *casually* add a command that isn't an environment
command).
> >
> > But I'm wondering whether it's rational to take it a step further --
do
> > something like insert the EnvName into the Run args, such that the
commands we
> > create/test/etc are not actually cmd.Commands -- thereby making it
> impossible(?)
> > to accidentally register an unwrapped env command.

> Not all commands operate on an environment. I started down this path
of having
> an alternative Run method (RunInEnv) which takes *cmd.Context and
envName, but
> found it too clumsy. The approach I've taken takes us in that
direction, so it
> could be changed with less churn later if desired.

> > FWIW, if it were structured that way, I'd be less dogmatic about the
need for
> > testing the list of registered commands...
> >
> > https://codereview.appspot.com/94350045/diff/1/cmd/juju/main.go
> > File cmd/juju/main.go (right):
> >
> >
https://codereview.appspot.com/94350045/diff/1/cmd/juju/main.go#newcode140
> > cmd/juju/main.go:140: jujucmd.Register(wrap(&cmd.VersionCommand{}))
> > Yeah, as Ian suggested, I'd really like us to be able to directly
test that we
> > actually get functional environment commands -- and ideally we'd get
this
> tested
> > on *every* registered command.

> I'll add a test that checks each registered command is an environment
command,
> with a list of exceptions.

Actually the test is simpler than that: it just checks that there are no
EnvironCommands registered. All EnvironCommands must be wrapped.

https://codereview.appspot.com/94350045/