Merge lp:~thumper/juju-core/support-string-slice-args into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Add gnuflag support for []string

Add a StringsValue type for gnuflag parsing.

This allows comma separated values to be used as a single arg, and they are
split up into the []string passed in to the NewStringsValue function.

https://codereview.appspot.com/39150045/

Add gnuflag support for []string

Add a StringsValue type for gnuflag parsing.

This allows comma separated values to be used as a single arg, and they are
split up into the []string passed in to the NewStringsValue function.

https://codereview.appspot.com/39150045/

[Tim Penhey (thumper)]

Reviewers: mp+199576_code.launchpad.net,

Message:
Please take a look.

Description:
Add gnuflag support for []string

Add a StringsValue type for gnuflag parsing.

This allows comma separated values to be used as a single arg, and they
are
split up into the []string passed in to the NewStringsValue function.

https://code.launchpad.net/~thumper/juju-core/support-string-slice-args/+merge/199576

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/39150045/

Affected files (+157, -0 lines):
   A [revision details]
   A cmd/args.go
   A cmd/args_test.go

[Roger Peppe (rogpeppe)]

Looks reasonable - some suggestions below.

https://codereview.appspot.com/39150045/diff/1/cmd/args.go
File cmd/args.go (right):

https://codereview.appspot.com/39150045/diff/1/cmd/args.go#newcode15
cmd/args.go:15: type StringsValue struct {
why not:

type StringsValue []string

?

https://codereview.appspot.com/39150045/diff/1/cmd/args.go#newcode21
cmd/args.go:21: func NewStringsValue(defaultValue []string, target
*[]string) *StringsValue {
If the type is as above, this function has less value.

https://codereview.appspot.com/39150045/diff/1/cmd/args.go#newcode31
cmd/args.go:31: trimmed := strings.TrimSpace(v)
I'm not sure I'd do this - I think it's not a bad thing to be able to
include spaces.

If you do decide it's necessary, this behaviour should at least be
documented.

https://codereview.appspot.com/39150045/diff/1/cmd/args.go#newcode32
cmd/args.go:32: if trimmed != "" {
I definitely wouldn't do this.
It's perfectly reasonable to want an empty item in a list.

https://codereview.appspot.com/39150045/

[Tim Penhey (thumper)]

Please take a look.

https://codereview.appspot.com/39150045/diff/1/cmd/args.go
File cmd/args.go (right):

https://codereview.appspot.com/39150045/diff/1/cmd/args.go#newcode15
cmd/args.go:15: type StringsValue struct {
On 2013/12/18 23:10:33, rog wrote:
> why not:

> type StringsValue []string

I ended up basing this on the constraints.Value work.

Using the type alias now.

https://codereview.appspot.com/39150045/diff/1/cmd/args.go#newcode21
cmd/args.go:21: func NewStringsValue(defaultValue []string, target
*[]string) *StringsValue {
On 2013/12/18 23:10:33, rog wrote:
> If the type is as above, this function has less value.

It is still necessary as it handles the cast and the assignment of
values.

https://codereview.appspot.com/39150045/diff/1/cmd/args.go#newcode31
cmd/args.go:31: trimmed := strings.TrimSpace(v)
On 2013/12/18 23:10:33, rog wrote:
> I'm not sure I'd do this - I think it's not a bad thing to be able to
include
> spaces.

> If you do decide it's necessary, this behaviour should at least be
documented.

well, it was only trimming off leading and trailing spaces, but fair
enough

https://codereview.appspot.com/39150045/diff/1/cmd/args.go#newcode32
cmd/args.go:32: if trimmed != "" {
On 2013/12/18 23:10:33, rog wrote:
> I definitely wouldn't do this.
> It's perfectly reasonable to want an empty item in a list.

/me shrugs

ok.

https://codereview.appspot.com/39150045/

[Roger Peppe (rogpeppe)]

LGTM, thanks.
One suggestion below.

https://codereview.appspot.com/39150045/diff/10004/cmd/args_test.go
File cmd/args_test.go (right):

https://codereview.appspot.com/39150045/diff/10004/cmd/args_test.go#newcode21
cmd/args_test.go:21: func (*ArgsSuite) TestNewStringsValue(c *gc.C) {
It'd be quite nice to see at least one test that tests it as it is
intended to be used (in a FlagSet). ISTM that this test could be easily
adapted to do that.

https://codereview.appspot.com/39150045/

=== added file 'cmd/args.go'
--- cmd/args.go	1970-01-01 00:00:00 +0000
+++ cmd/args.go	2013-12-19 22:35:21 +0000
@@ -0,0 +1,36 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package cmd
+
+import (
+	"strings"
+
+	"launchpad.net/gnuflag"
+)
+
+// StringsValue implements gnuflag.Value for a comma separated list of
+// strings.  This allows flags to be created where the target is []string, and
+// the caller is after comma separated values.
+type StringsValue []string
+
+var _ gnuflag.Value = (*StringsValue)(nil)
+
+// NewStringsValue is used to create the type passed into the gnuflag.FlagSet Var function.
+// f.Var(cmd.NewStringsValue(defaultValue, &someMember), "name", "help")
+func NewStringsValue(defaultValue []string, target *[]string) *StringsValue {
+	value := (*StringsValue)(target)
+	*value = defaultValue
+	return value
+}
+
+// Implements gnuflag.Value Set.
+func (v *StringsValue) Set(s string) error {
+	*v = strings.Split(s, ",")
+	return nil
+}
+
+// Implements gnuflag.Value String.
+func (v *StringsValue) String() string {
+	return strings.Join(*v, ",")
+}

=== added file 'cmd/args_test.go'
--- cmd/args_test.go	1970-01-01 00:00:00 +0000
+++ cmd/args_test.go	2013-12-19 22:35:21 +0000
@@ -0,0 +1,151 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package cmd_test
+
+import (
+	"fmt"
+	"io/ioutil"
+
+	"launchpad.net/gnuflag"
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/cmd"
+	"launchpad.net/juju-core/testing/testbase"
+)
+
+type ArgsSuite struct {
+	testbase.LoggingSuite
+}
+
+var _ = gc.Suite(&ArgsSuite{})
+
+func (*ArgsSuite) TestFlagsUsage(c *gc.C) {
+	for i, test := range []struct {
+		message       string
+		defaultValue  []string
+		args          []string
+		expectedValue []string
+	}{{
+		message: "nil default and no arg",
+	}, {
+		message:       "default value and not set by args",
+		defaultValue:  []string{"foo", "bar"},
+		expectedValue: []string{"foo", "bar"},
+	}, {
+		message:       "no value set by args",
+		args:          []string{"--value", "foo,bar"},
+		expectedValue: []string{"foo", "bar"},
+	}, {
+		message:       "default value and set by args",
+		defaultValue:  []string{"omg"},
+		args:          []string{"--value", "foo,bar"},
+		expectedValue: []string{"foo", "bar"},
+	}} {
+		c.Log(fmt.Sprintf("%v: %s", i, test.message))
+		f := gnuflag.NewFlagSet("test", gnuflag.ContinueOnError)
+		f.SetOutput(ioutil.Discard)
+		var value []string
+		f.Var(cmd.NewStringsValue(test.defaultValue, &value), "value", "help")
+		err := f.Parse(false, test.args)
+		c.Check(err, gc.IsNil)
+		c.Check(value, gc.DeepEquals, test.expectedValue)
+	}
+}
+
+func (*ArgsSuite) TestNewStringsValue(c *gc.C) {
+	for i, test := range []struct {
+		message      string
+		defaultValue []string
+	}{{
+		message: "null default",
+	}, {
+		message:      "empty default",
+		defaultValue: []string{},
+	}, {
+		message:      "single value",
+		defaultValue: []string{"foo"},
+	}, {
+		message:      "multiple values",
+		defaultValue: []string{"foo", "bar", "baz"},
+	}} {
+		c.Log(fmt.Sprintf("%v: %s", i, test.message))
+		var underlyingValue []string
+		_ = cmd.NewStringsValue(test.defaultValue, &underlyingValue)
+		c.Assert(underlyingValue, gc.DeepEquals, test.defaultValue)
+	}
+}
+
+func (*ArgsSuite) TestSet(c *gc.C) {
+	for i, test := range []struct {
+		message  string
+		arg      string
+		expected []string
+	}{{
+		message:  "empty",
+		expected: []string{""},
+	}, {
+		message:  "just whitespace",
+		arg:      "   ",
+		expected: []string{"   "},
+	}, {
+		message:  "whitespace and comma",
+		arg:      "  ,  ",
+		expected: []string{"  ", "  "},
+	}, {
+		message:  "single value",
+		arg:      "foo",
+		expected: []string{"foo"},
+	}, {
+		message:  "single value with comma",
+		arg:      "foo,",
+		expected: []string{"foo", ""},
+	}, {
+		message:  "single value with whitespace",
+		arg:      " foo ",
+		expected: []string{" foo "},
+	}, {
+		message:  "multiple values",
+		arg:      "foo,bar,baz",
+		expected: []string{"foo", "bar", "baz"},
+	}, {
+		message:  "multiple values with spaces",
+		arg:      "foo, bar, baz",
+		expected: []string{"foo", " bar", " baz"},
+	}} {
+		c.Log(fmt.Sprintf("%v: %s", i, test.message))
+		var result []string
+		value := cmd.NewStringsValue(nil, &result)
+		error := value.Set(test.arg)
+		c.Check(error, gc.IsNil)
+		c.Check(result, gc.DeepEquals, test.expected)
+	}
+}
+
+func (*ArgsSuite) TestString(c *gc.C) {
+	for i, test := range []struct {
+		message  string
+		target   []string
+		expected string
+	}{{
+		message:  "null",
+		expected: "",
+	}, {
+		message:  "empty",
+		target:   []string{},
+		expected: "",
+	}, {
+		message:  "single value",
+		target:   []string{"foo"},
+		expected: "foo",
+	}, {
+		message:  "multiple values",
+		target:   []string{"foo", "bar", "baz"},
+		expected: "foo,bar,baz",
+	}} {
+		c.Log(fmt.Sprintf("%v: %s", i, test.message))
+		var temp []string
+		value := cmd.NewStringsValue(test.target, &temp)
+		c.Assert(value.String(), gc.Equals, test.expected)
+	}
+}