juju-core

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

  1. support-string-slice-args
  2. Merge into trunk
Proposed by Tim Penhey
Status: Merged
Approved by: Tim Penhey
Approved revision: no longer in the source branch.
Merged at revision: 2166
Proposed branch: lp:~thumper/juju-core/support-string-slice-args
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 196 lines (+187/-0)
2 files modified
cmd/args.go (+36/-0)
cmd/args_test.go (+151/-0)
To merge this branch: bzr merge lp:~thumper/juju-core/support-string-slice-args
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+199576@code.launchpad.net

Commit message

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/

Description of the change

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/

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

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

Revision history for this message
Roger Peppe (rogpeppe) wrote :

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/

Revision history for this message
Tim Penhey (thumper) wrote :

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/

Revision history for this message
Roger Peppe (rogpeppe) wrote :

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/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
1// Copyright 2013 Canonical Ltd.
2// Licensed under the AGPLv3, see LICENCE file for details.
3
4package cmd
5
6import (
7 "strings"
8
9 "launchpad.net/gnuflag"
10)
11
12// StringsValue implements gnuflag.Value for a comma separated list of
13// strings. This allows flags to be created where the target is []string, and
14// the caller is after comma separated values.
15type StringsValue []string
16
17var _ gnuflag.Value = (*StringsValue)(nil)
18
19// NewStringsValue is used to create the type passed into the gnuflag.FlagSet Var function.
20// f.Var(cmd.NewStringsValue(defaultValue, &someMember), "name", "help")
21func NewStringsValue(defaultValue []string, target *[]string) *StringsValue {
22 value := (*StringsValue)(target)
23 *value = defaultValue
24 return value
25}
26
27// Implements gnuflag.Value Set.
28func (v *StringsValue) Set(s string) error {
29 *v = strings.Split(s, ",")
30 return nil
31}
32
33// Implements gnuflag.Value String.
34func (v *StringsValue) String() string {
35 return strings.Join(*v, ",")
36}
037
=== 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 @@
1// Copyright 2013 Canonical Ltd.
2// Licensed under the AGPLv3, see LICENCE file for details.
3
4package cmd_test
5
6import (
7 "fmt"
8 "io/ioutil"
9
10 "launchpad.net/gnuflag"
11 gc "launchpad.net/gocheck"
12
13 "launchpad.net/juju-core/cmd"
14 "launchpad.net/juju-core/testing/testbase"
15)
16
17type ArgsSuite struct {
18 testbase.LoggingSuite
19}
20
21var _ = gc.Suite(&ArgsSuite{})
22
23func (*ArgsSuite) TestFlagsUsage(c *gc.C) {
24 for i, test := range []struct {
25 message string
26 defaultValue []string
27 args []string
28 expectedValue []string
29 }{{
30 message: "nil default and no arg",
31 }, {
32 message: "default value and not set by args",
33 defaultValue: []string{"foo", "bar"},
34 expectedValue: []string{"foo", "bar"},
35 }, {
36 message: "no value set by args",
37 args: []string{"--value", "foo,bar"},
38 expectedValue: []string{"foo", "bar"},
39 }, {
40 message: "default value and set by args",
41 defaultValue: []string{"omg"},
42 args: []string{"--value", "foo,bar"},
43 expectedValue: []string{"foo", "bar"},
44 }} {
45 c.Log(fmt.Sprintf("%v: %s", i, test.message))
46 f := gnuflag.NewFlagSet("test", gnuflag.ContinueOnError)
47 f.SetOutput(ioutil.Discard)
48 var value []string
49 f.Var(cmd.NewStringsValue(test.defaultValue, &value), "value", "help")
50 err := f.Parse(false, test.args)
51 c.Check(err, gc.IsNil)
52 c.Check(value, gc.DeepEquals, test.expectedValue)
53 }
54}
55
56func (*ArgsSuite) TestNewStringsValue(c *gc.C) {
57 for i, test := range []struct {
58 message string
59 defaultValue []string
60 }{{
61 message: "null default",
62 }, {
63 message: "empty default",
64 defaultValue: []string{},
65 }, {
66 message: "single value",
67 defaultValue: []string{"foo"},
68 }, {
69 message: "multiple values",
70 defaultValue: []string{"foo", "bar", "baz"},
71 }} {
72 c.Log(fmt.Sprintf("%v: %s", i, test.message))
73 var underlyingValue []string
74 _ = cmd.NewStringsValue(test.defaultValue, &underlyingValue)
75 c.Assert(underlyingValue, gc.DeepEquals, test.defaultValue)
76 }
77}
78
79func (*ArgsSuite) TestSet(c *gc.C) {
80 for i, test := range []struct {
81 message string
82 arg string
83 expected []string
84 }{{
85 message: "empty",
86 expected: []string{""},
87 }, {
88 message: "just whitespace",
89 arg: " ",
90 expected: []string{" "},
91 }, {
92 message: "whitespace and comma",
93 arg: " , ",
94 expected: []string{" ", " "},
95 }, {
96 message: "single value",
97 arg: "foo",
98 expected: []string{"foo"},
99 }, {
100 message: "single value with comma",
101 arg: "foo,",
102 expected: []string{"foo", ""},
103 }, {
104 message: "single value with whitespace",
105 arg: " foo ",
106 expected: []string{" foo "},
107 }, {
108 message: "multiple values",
109 arg: "foo,bar,baz",
110 expected: []string{"foo", "bar", "baz"},
111 }, {
112 message: "multiple values with spaces",
113 arg: "foo, bar, baz",
114 expected: []string{"foo", " bar", " baz"},
115 }} {
116 c.Log(fmt.Sprintf("%v: %s", i, test.message))
117 var result []string
118 value := cmd.NewStringsValue(nil, &result)
119 error := value.Set(test.arg)
120 c.Check(error, gc.IsNil)
121 c.Check(result, gc.DeepEquals, test.expected)
122 }
123}
124
125func (*ArgsSuite) TestString(c *gc.C) {
126 for i, test := range []struct {
127 message string
128 target []string
129 expected string
130 }{{
131 message: "null",
132 expected: "",
133 }, {
134 message: "empty",
135 target: []string{},
136 expected: "",
137 }, {
138 message: "single value",
139 target: []string{"foo"},
140 expected: "foo",
141 }, {
142 message: "multiple values",
143 target: []string{"foo", "bar", "baz"},
144 expected: "foo,bar,baz",
145 }} {
146 c.Log(fmt.Sprintf("%v: %s", i, test.message))
147 var temp []string
148 value := cmd.NewStringsValue(test.target, &temp)
149 c.Assert(value.String(), gc.Equals, test.expected)
150 }
151}

Subscribers

People subscribed via source and target branches

to status/vote changes: