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

Revision history for this message
William Reade (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.

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.

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

« Back to merge proposal