Add gnuflag support for []string

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)

[thumper, 2013-12-18 22:07:01]

Please take a look.

[rog, 2013-12-18 23:10:33]

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.

[thumper, 2013-12-19 09:33:00]

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.

[rog, 2013-12-19 10:57:23]

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.