|
|
|
switch: more script friendly output
Changed switch command to more script friendly
output. Branch has been newly created after an
error when reverting.
https://code.launchpad.net/~themue/juju-core/054-env-more-script-friendly/+merge/191838
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 4
Patch Set 2 : switch: more script friendly output #
Total messages: 11
|
mue
Please take a look.
|
12 years, 2 months ago (2013年10月18日 16:49:49 UTC) #1 | ||||||||||||||||||||||||||||||||
Please take a look.
Code looks fine apart from one possibly spurious struct member addition. I have mixed feelings about making switch less user friendly and more script friendly. It feels mostly like a user-only command to me, unlike status say. In bzr it's similar, status gives output that's quite machine parsable, but switch is an interactive command. In the bug, stub doesn't really go into *why* he wants to script this, my instinct would be to say using seperate juju homes would be more suitable than having one and switching. https://codereview.appspot.com/15080044/diff/1/cmd/juju/switch.go File cmd/juju/switch.go (right): https://codereview.appspot.com/15080044/diff/1/cmd/juju/switch.go#newcode22 cmd/juju/switch.go:22: Raw bool What's this used for?
On 2013年10月18日 17:41:49, gz wrote: > Code looks fine apart from one possibly spurious struct member addition. > > I have mixed feelings about making switch less user friendly and more script > friendly. It feels mostly like a user-only command to me, unlike status say. In > bzr it's similar, status gives output that's quite machine parsable, but switch > is an interactive command. In the bug, stub doesn't really go into *why* he > wants to script this, my instinct would be to say using seperate juju homes > would be more suitable than having one and switching. > > https://codereview.appspot.com/15080044/diff/1/cmd/juju/switch.go > File cmd/juju/switch.go (right): > > https://codereview.appspot.com/15080044/diff/1/cmd/juju/switch.go#newcode22 > cmd/juju/switch.go:22: Raw bool > What's this used for? Ooops, the raw has indeed to be removed, it's a leftover of a former try. The discussion about the output has gone through several circles, with --raw and raw as default. This is the agreement after the discussion. The problem is that this command is overloaded. It gets, sets and lists for the user and for scripts.
LGTM with one thought below. Personally I think that the script friendly output is also user friendly - verbosity does not necessarily imply friendliness. Also many people write ad-hoc scripts on the command line - it's good to be an upstanding command line citizen and play nicely with the shell IMHO. https://codereview.appspot.com/15080044/diff/1/cmd/juju/switch.go File cmd/juju/switch.go (right): https://codereview.appspot.com/15080044/diff/1/cmd/juju/switch.go#newcode102 cmd/juju/switch.go:102: fmt.Fprintf(ctx.Stdout, "%s (*)\n", name) I'm not sure it's worth showing this. It means that this won't work, for example: for i in `juju switch -l`; do juju status -e $i done I'm not sure we need to combine the two functionalities (list environments and print current environment name). Let's keep it as simple as possible. YMMV though.
This looks like a perfect situation for a --format param and some structs, and leaving the existing output unchanged as the "smart" (haha) option. What led us do do it otherwise?
On 2013年11月18日 10:46:13, william.reade wrote: > This looks like a perfect situation for a --format param and some structs, and > leaving the existing output unchanged as the "smart" (haha) option. What led us > do do it otherwise? There has been a first approach: https://code.launchpad.net/~themue/juju-core/053-env-more-script-friendly The reviews and the discussion for it led to a second approach. Different people have different preferences. I'll talk to Stuart about his preferences, he "owns" the issue.
On 18 November 2013 10:46, <william.reade@canonical.com> wrote: > This looks like a perfect situation for a --format param and some > structs, and leaving the existing output unchanged as the "smart" (haha) > option. What led us do do it otherwise? This was a very new command. I thought that changing it now would result in cleaner code and a somewhat slicker interface with minimal compatibility issues. I don't think the verbose output adds any particular value. This was all discussed quite a while ago.
On 2013年11月18日 14:05:05, rog wrote: > On 18 November 2013 10:46, <mailto:william.reade@canonical.com> wrote: > > This looks like a perfect situation for a --format param and some > > structs, and leaving the existing output unchanged as the "smart" (haha) > > option. What led us do do it otherwise? > > This was a very new command. > > I thought that changing it now would result in cleaner > code and a somewhat slicker interface with minimal > compatibility issues. I don't think the verbose > output adds any particular value. > > This was all discussed quite a while ago. Fair enough, I guess I just missed that one. Thanks for the history. I'm +1 on your drop-the-(*) suggestion FWIW.
On 2013年11月18日 14:16:40, william.reade wrote: > On 2013年11月18日 14:05:05, rog wrote: > > On 18 November 2013 10:46, <mailto:william.reade@canonical.com> wrote: > > > This looks like a perfect situation for a --format param and some > > > structs, and leaving the existing output unchanged as the "smart" (haha) > > > option. What led us do do it otherwise? > > > > This was a very new command. > > > > I thought that changing it now would result in cleaner > > code and a somewhat slicker interface with minimal > > compatibility issues. I don't think the verbose > > output adds any particular value. > > > > This was all discussed quite a while ago. > > Fair enough, I guess I just missed that one. Thanks for the history. I'm +1 on > your drop-the-(*) suggestion FWIW. Fine, then I'll do it that way.
https://codereview.appspot.com/15080044/diff/1/cmd/juju/switch.go File cmd/juju/switch.go (right): https://codereview.appspot.com/15080044/diff/1/cmd/juju/switch.go#newcode22 cmd/juju/switch.go:22: Raw bool On 2013年10月18日 17:41:49, gz wrote: > What's this used for? Ouch, old approach, missed to remove it. Thx. https://codereview.appspot.com/15080044/diff/1/cmd/juju/switch.go#newcode102 cmd/juju/switch.go:102: fmt.Fprintf(ctx.Stdout, "%s (*)\n", name) On 2013年11月12日 16:11:27, rog wrote: > I'm not sure it's worth showing this. > It means that this won't work, for example: > > for i in `juju switch -l`; do > juju status -e $i > done > > I'm not sure we need to combine the two > functionalities (list environments and print > current environment name). Let's keep it as > simple as possible. > > YMMV though. Done.