Add hopefully-reusable --output and --format flags to config-get

Add hopefully-reusable --output and --format flags to config-get

Format defaults to "smart", as in python, but does not produce the same
output: it produces go-specific stringifications rather than python-specific
ones. Since "smart" python output was clearly never intended to be (easily)
machine-readable (or it would have used repr) this has been agreed to be
equivalent enough.

https://code.launchpad.net/~fwereade/juju/go-config-get-command-flags/+merge/104410

Requires: https://code.launchpad.net/~fwereade/juju/go-config-get-command-start/+merge/104370

(do not edit description out of merge proposal)

[fwereade, 2012-05-02 16:08:20]

Please take a look.

[rog, 2012-05-02 17:05:52]

i appreciate the idea, and i was initially +1, but after reflection i'm not sure
the abstraction pulls its weight. the python juju doesn't have -o or --output
flags for config-get and i don't think it needs them (that's why we've got shell
redirection :-]).

the only other command in the python juju that this would seem to be useful for
is juju-status, but i'm not convinced this would make it easier (you'd lose the
static typing because of the interface{}-typed converter function too).

i am wondering if a type to make it easier to specify a list of possible values
to a flag might be a better fit here; or just do the output kind check after the
flag parsing and don't worry too much about the exit code.

here's the kind of thing i mean by "a type to make it easier to specify a list
of possible values to a flag":

type oneOfValue struct {
	what     string
	possible []string
	value    string
}

// newOneOfValue returns an implementation of gnuflag.Value
// that will allow the flag value to take any of the listed
// possible values.
func newOneOfValue(what string, possible ...string) *oneOfValue {
	return &oneOfValue{
		what:     what,
		possible: possible,
	}
}

func (v *oneOfValue) Set(value string) error {
	for _, s := range v.possible {
		if s == value {
			v.value = value
			return nil
		}
	}
	return errors.New("unknown " + v.what)
}

func (v *oneOfValue) String() string {
	return v.value
}

https://codereview.appspot.com/6165044/diff/1/cmd/jujuc/server/output.go
File cmd/jujuc/server/output.go (right):

https://codereview.appspot.com/6165044/diff/1/cmd/jujuc/server/output.go#newc...
cmd/jujuc/server/output.go:15: type converters map[string]converter
how useful is this general functionality? are we going to have all kinds of
different converters? i'm wondering if we might want to just hardwire json and
smart...

https://codereview.appspot.com/6165044/diff/1/cmd/jujuc/server/output.go#newc...
cmd/jujuc/server/output.go:29: value      string
i think this would be better named "name", as it's the name of the converter.

https://codereview.appspot.com/6165044/diff/1/cmd/jujuc/server/output.go#newc...
cmd/jujuc/server/output.go:45: if _, found := v.converters[value]; !found {
if v.converters[value] == nil {

should be sufficient

https://codereview.appspot.com/6165044/diff/1/cmd/jujuc/server/output.go#newc...
cmd/jujuc/server/output.go:66: return "specify output format (%s)" +
strings.Join(choices, "|")
"specify output format (" + strings.Join(choices, "|") + ")"

https://codereview.appspot.com/6165044/diff/1/cmd/jujuc/server/output.go#newc...
cmd/jujuc/server/output.go:71: func (v *converterValue) write(target io.Writer,
value interface{}) (err error) {
i'm not convinced that the extra layer of indirection is worth it. i'd think i'd
put this code into resultWriter.write, then all the actual conversion logic is
in one place.

https://codereview.appspot.com/6165044/diff/1/cmd/jujuc/server/output.go#newc...
cmd/jujuc/server/output.go:105: if target, err = os.OpenFile(path,
os.O_WRONLY|os.O_CREATE, 0644); err != nil {
os.Create

[fwereade, 2012-05-02 17:14:52]

On 2012/05/02 17:05:52, rog wrote:
> i appreciate the idea, and i was initially +1, but after reflection i'm not
sure
> the abstraction pulls its weight. the python juju doesn't have -o or --output
> flags for config-get and i don't think it needs them (that's why we've got
shell
> redirection :-]).

juju/hooks/cli/py:54

I agree that shell redirection should cover this, but I'm -1 on still *more*
changes to the API; even the changes to the useless logging flags made me a bit
uncomfortable.

[more to come, just wanted to note that]

[fwereade, 2012-05-02 17:27:44]

Please take a look.

https://codereview.appspot.com/6165044/diff/1/cmd/jujuc/server/output.go
File cmd/jujuc/server/output.go (right):

https://codereview.appspot.com/6165044/diff/1/cmd/jujuc/server/output.go#newc...
cmd/jujuc/server/output.go:15: type converters map[string]converter
On 2012/05/02 17:05:52, rog wrote:
> how useful is this general functionality? are we going to have all kinds of
> different converters? i'm wondering if we might want to just hardwire json and
> smart...

There are 6 different ones, 3 of them called "smart".

https://codereview.appspot.com/6165044/diff/1/cmd/jujuc/server/output.go#newc...
cmd/jujuc/server/output.go:29: value      string
On 2012/05/02 17:05:52, rog wrote:
> i think this would be better named "name", as it's the name of the converter.

Done.

https://codereview.appspot.com/6165044/diff/1/cmd/jujuc/server/output.go#newc...
cmd/jujuc/server/output.go:45: if _, found := v.converters[value]; !found {
On 2012/05/02 17:05:52, rog wrote:
> if v.converters[value] == nil {
> 
> should be sufficient

Done.

https://codereview.appspot.com/6165044/diff/1/cmd/jujuc/server/output.go#newc...
cmd/jujuc/server/output.go:66: return "specify output format (%s)" +
strings.Join(choices, "|")
On 2012/05/02 17:05:52, rog wrote:
> "specify output format (" + strings.Join(choices, "|") + ")"

Done. Looks like I forgot to test the output :/.

https://codereview.appspot.com/6165044/diff/1/cmd/jujuc/server/output.go#newc...
cmd/jujuc/server/output.go:71: func (v *converterValue) write(target io.Writer,
value interface{}) (err error) {
On 2012/05/02 17:05:52, rog wrote:
> i'm not convinced that the extra layer of indirection is worth it. i'd think
i'd
> put this code into resultWriter.write, then all the actual conversion logic is
> in one place.

It feels to me as though putting *everything* in one place is a bit heavyweight;
and if we want to separate the output file choice from the format choice, I
think this is the right division of responsibilities.

If we decide to drop --output altogether that's another matter.

https://codereview.appspot.com/6165044/diff/1/cmd/jujuc/server/output.go#newc...
cmd/jujuc/server/output.go:105: if target, err = os.OpenFile(path,
os.O_WRONLY|os.O_CREATE, 0644); err != nil {
On 2012/05/02 17:05:52, rog wrote:
> os.Create

Done.

https://codereview.appspot.com/6165044/diff/1/cmd/jujuc/server/output.go#newc...
cmd/jujuc/server/output.go:105: if target, err = os.OpenFile(path,
os.O_WRONLY|os.O_CREATE, 0644); err != nil {
On 2012/05/02 17:05:52, rog wrote:
> os.Create

Done.

[rog, 2012-05-02 17:47:48]

On 2012/05/02 17:14:52, fwereade wrote:
> On 2012/05/02 17:05:52, rog wrote:
> > i appreciate the idea, and i was initially +1, but after reflection i'm not
> sure
> > the abstraction pulls its weight. the python juju doesn't have -o or
--output
> > flags for config-get and i don't think it needs them (that's why we've got
> shell
> > redirection :-]).
> 
> juju/hooks/cli/py:54
> 
> I agree that shell redirection should cover this, but I'm -1 on still *more*
> changes to the API; even the changes to the useless logging flags made me a
bit
> uncomfortable.
> 
> [more to come, just wanted to note that]

ah yes, i hadn't spotted that. i see where you're coming from now.

if we're going for backward compatibility, we should ensure that *all* hook
commands accept --format and --output flags.

i'm still not sure that this general approach to formatting will be nicer than
simply switching on the format type in Run in each command.
it's the disconnect between data and formatter that bothers me - a formatting
function will look general, but will actually only work on a specific set of
data types particular to a given command.

i'll continue thinking about it.

[rog, 2012-05-02 17:50:39]

https://codereview.appspot.com/6165044/diff/1/cmd/jujuc/server/output.go
File cmd/jujuc/server/output.go (right):

https://codereview.appspot.com/6165044/diff/1/cmd/jujuc/server/output.go#newc...
cmd/jujuc/server/output.go:71: func (v *converterValue) write(target io.Writer,
value interface{}) (err error) {
On 2012/05/02 17:27:44, fwereade wrote:
> On 2012/05/02 17:05:52, rog wrote:
> > i'm not convinced that the extra layer of indirection is worth it. i'd think
> i'd
> > put this code into resultWriter.write, then all the actual conversion logic
is
> > in one place.
> 
> It feels to me as though putting *everything* in one place is a bit
heavyweight;
> and if we want to separate the output file choice from the format choice, I
> think this is the right division of responsibilities.

*if* we do, then i agree. but in the proposed code it just seemed like a little
bit of extra mental baggage to me.

> If we decide to drop --output altogether that's another matter.

i agree with your reasoning that we want to keep backward compatibility.

[niemeyer, 2012-05-03 01:18:47]

LGTM, assuming the following is sorted:

https://codereview.appspot.com/6165044/diff/1006/cmd/jujuc/server/config-get.go
File cmd/jujuc/server/config-get.go (right):

https://codereview.appspot.com/6165044/diff/1006/cmd/jujuc/server/config-get....
cmd/jujuc/server/config-get.go:37: "smart": convertSmart,
This dictionary should probably be a variable on output.go:

var defaultConverters = ...

https://codereview.appspot.com/6165044/diff/1006/cmd/jujuc/server/output.go
File cmd/jujuc/server/output.go (right):

https://codereview.appspot.com/6165044/diff/1006/cmd/jujuc/server/output.go#n...
cmd/jujuc/server/output.go:15: type converters map[string]converter
The converters type isn't necessary. It's just obscuring the code below without
much benefit, IMO.

https://codereview.appspot.com/6165044/diff/1006/cmd/jujuc/server/output.go#n...
cmd/jujuc/server/output.go:24: return []byte(fmt.Sprintf("%v", value)), nil
fmt.Sprint(value) works, and I think it preserves equivalent semantics to the
Python version. If something implements String, it will be stringified
accordingly. I suggest using this, removing the comment internal to the
function, and improving the comment at the top of the function to explain what
it actually does.

https://codereview.appspot.com/6165044/diff/1006/cmd/jujuc/server/output.go#n...
cmd/jujuc/server/output.go:71: func (v *converterValue) write(target io.Writer,
value interface{}) (err error) {
This looks a bit misplaced. The scope of converterValue is command line
management. What do you think of:

func (c converter) write(target io.Writer, value interface{}) error {

https://codereview.appspot.com/6165044/diff/1006/cmd/jujuc/server/output.go#n...
cmd/jujuc/server/output.go:83: // resultWriter is responsible for writing
command output, and exposes flags
It'd be nice to expand that explanation a bit (hint: fmt.Printf is also
responsible for writing command output).

[niemeyer, 2012-05-03 01:24:48]

https://codereview.appspot.com/6165044/diff/1006/cmd/jujuc/server/output.go
File cmd/jujuc/server/output.go (right):

https://codereview.appspot.com/6165044/diff/1006/cmd/jujuc/server/output.go#n...
cmd/jujuc/server/output.go:71: func (v *converterValue) write(target io.Writer,
value interface{}) (err error) {
On 2012/05/03 01:18:47, niemeyer wrote:
> This looks a bit misplaced. The scope of converterValue is command line
> management. What do you think of:
> 
> func (c converter) write(target io.Writer, value interface{}) error {

Actually, Roger's comment sounds even more sensible to me. This is on the
trivial side, and is used just once at the end of a method. Having it inside the
other write method sounds good enough.

https://codereview.appspot.com/6165044/diff/1006/cmd/jujuc/server/output.go#n...
cmd/jujuc/server/output.go:78: _, err = target.Write(bytes)
This might also be simplified to avoid copying the whole bytes array just to
append an endline:

_, err = target.Write(bytes)
if err == nil {
    _, err = target.Write([]byte{'\n'})
}

[fwereade, 2012-05-03 10:26:45]

Please take a look.

https://codereview.appspot.com/6165044/diff/1006/cmd/jujuc/server/config-get.go
File cmd/jujuc/server/config-get.go (right):

https://codereview.appspot.com/6165044/diff/1006/cmd/jujuc/server/config-get....
cmd/jujuc/server/config-get.go:37: "smart": convertSmart,
On 2012/05/03 01:18:47, niemeyer wrote:
> This dictionary should probably be a variable on output.go:
> 
> var defaultConverters = ...

Done, for now, but I suspect we'll need to explode it again once we've
implemented a few more commands.

https://codereview.appspot.com/6165044/diff/1006/cmd/jujuc/server/output.go
File cmd/jujuc/server/output.go (right):

https://codereview.appspot.com/6165044/diff/1006/cmd/jujuc/server/output.go#n...
cmd/jujuc/server/output.go:15: type converters map[string]converter
On 2012/05/03 01:18:47, niemeyer wrote:
> The converters type isn't necessary. It's just obscuring the code below
without
> much benefit, IMO.

Done.

https://codereview.appspot.com/6165044/diff/1006/cmd/jujuc/server/output.go#n...
cmd/jujuc/server/output.go:24: return []byte(fmt.Sprintf("%v", value)), nil
On 2012/05/03 01:18:47, niemeyer wrote:
> fmt.Sprint(value) works, and I think it preserves equivalent semantics to the
> Python version. If something implements String, it will be stringified
> accordingly. I suggest using this, removing the comment internal to the
> function, and improving the comment at the top of the function to explain what
> it actually does.

Done.

https://codereview.appspot.com/6165044/diff/1006/cmd/jujuc/server/output.go#n...
cmd/jujuc/server/output.go:71: func (v *converterValue) write(target io.Writer,
value interface{}) (err error) {
On 2012/05/03 01:24:48, niemeyer wrote:
> On 2012/05/03 01:18:47, niemeyer wrote:
> > This looks a bit misplaced. The scope of converterValue is command line
> > management. What do you think of:
> > 
> > func (c converter) write(target io.Writer, value interface{}) error {
> 
> Actually, Roger's comment sounds even more sensible to me. This is on the
> trivial side, and is used just once at the end of a method. Having it inside
the
> other write method sounds good enough.

Done.

https://codereview.appspot.com/6165044/diff/1006/cmd/jujuc/server/output.go#n...
cmd/jujuc/server/output.go:78: _, err = target.Write(bytes)
On 2012/05/03 01:24:48, niemeyer wrote:
> This might also be simplified to avoid copying the whole bytes array just to
> append an endline:
> 
> _, err = target.Write(bytes)
> if err == nil {
>     _, err = target.Write([]byte{'\n'})
> }

Done.

https://codereview.appspot.com/6165044/diff/1006/cmd/jujuc/server/output.go#n...
cmd/jujuc/server/output.go:83: // resultWriter is responsible for writing
command output, and exposes flags
On 2012/05/03 01:18:47, niemeyer wrote:
> It'd be nice to expand that explanation a bit (hint: fmt.Printf is also
> responsible for writing command output).

Done.

[niemeyer, 2012-05-03 17:46:50]

LGTM, thanks. A trivial detail, FWIW:

https://codereview.appspot.com/6165044/diff/12001/cmd/jujuc/server/output.go
File cmd/jujuc/server/output.go (right):

https://codereview.appspot.com/6165044/diff/12001/cmd/jujuc/server/output.go#...
cmd/jujuc/server/output.go:106: var bytes []byte
That's not necessary. Just do "bytes, err :=" below and it will work properly
with everything else unchanged.

[fwereade, 2012-05-04 08:05:15]

*** Submitted:

Add hopefully-reusable --output and --format flags to config-get

Format defaults to "smart", as in python, but does not produce the same
output: it produces go-specific stringifications rather than python-specific
ones. Since "smart" python output was clearly never intended to be (easily)
machine-readable (or it would have used repr) this has been agreed to be
equivalent enough.

R=rog, niemeyer
CC=
https://codereview.appspot.com/6165044

https://codereview.appspot.com/6165044/diff/12001/cmd/jujuc/server/output.go
File cmd/jujuc/server/output.go (right):

https://codereview.appspot.com/6165044/diff/12001/cmd/jujuc/server/output.go#...
cmd/jujuc/server/output.go:106: var bytes []byte
On 2012/05/03 17:46:50, niemeyer wrote:
> That's not necessary. Just do "bytes, err :=" below and it will work properly
> with everything else unchanged.

Done.