Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

StringsOption with delimiter #204

Merged
merged 6 commits into from
Jan 27, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cli/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ func setOpts(kv kv, kvType reflect.Kind, opts cmds.OptMap) error {

if kvType == cmds.Strings {
res, _ := opts[kv.Key].([]string)
opts[kv.Key] = append(res, kv.Value.(string))
opts[kv.Key] = append(res, kv.Value.([]string)...)
} else if _, exists := opts[kv.Key]; !exists {
opts[kv.Key] = kv.Value
} else {
Expand Down
190 changes: 170 additions & 20 deletions cli/parse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,32 @@ func TestSameWords(t *testing.T) {
test(f, f, true)
}

func testOptionHelper(t *testing.T, cmd *cmds.Command, args string, expectedOpts kvs, expectedWords words, expectErr bool) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was just extracted from the function below

req := &cmds.Request{}
err := parse(req, strings.Split(args, " "), cmd)
if err == nil {
err = req.FillDefaults()
}
if expectErr {
if err == nil {
t.Errorf("Command line '%v' parsing should have failed", args)
}
} else if err != nil {
t.Errorf("Command line '%v' failed to parse: %v", args, err)
} else if !sameWords(req.Arguments, expectedWords) || !sameKVs(kvs(req.Options), expectedOpts) {
t.Errorf("Command line '%v':\n parsed as %v %v\n instead of %v %v",
args, req.Options, req.Arguments, expectedOpts, expectedWords)
}
}

func TestOptionParsing(t *testing.T) {
cmd := &cmds.Command{
Options: []cmds.Option{
cmds.StringOption("string", "s", "a string"),
cmds.StringOption("flag", "alias", "multiple long"),
cmds.BoolOption("bool", "b", "a bool"),
cmds.StringsOption("strings", "r", "strings array"),
cmds.DelimitedStringsOption(",", "delimstrings", "d", "comma delimited string array"),
},
Subcommands: map[string]*cmds.Command{
"test": &cmds.Command{},
Expand All @@ -95,30 +114,12 @@ func TestOptionParsing(t *testing.T) {
},
}

testHelper := func(args string, expectedOpts kvs, expectedWords words, expectErr bool) {
req := &cmds.Request{}
err := parse(req, strings.Split(args, " "), cmd)
if err == nil {
err = req.FillDefaults()
}
if expectErr {
if err == nil {
t.Errorf("Command line '%v' parsing should have failed", args)
}
} else if err != nil {
t.Errorf("Command line '%v' failed to parse: %v", args, err)
} else if !sameWords(req.Arguments, expectedWords) || !sameKVs(kvs(req.Options), expectedOpts) {
t.Errorf("Command line '%v':\n parsed as %v %v\n instead of %v %v",
args, req.Options, req.Arguments, expectedOpts, expectedWords)
}
}

testFail := func(args string) {
testHelper(args, kvs{}, words{}, true)
testOptionHelper(t, cmd, args, kvs{}, words{}, true)
}

test := func(args string, expectedOpts kvs, expectedWords words) {
testHelper(args, expectedOpts, expectedWords, false)
testOptionHelper(t, cmd, args, expectedOpts, expectedWords, false)
}

test("test -", kvs{}, words{"-"})
Expand Down Expand Up @@ -154,6 +155,13 @@ func TestOptionParsing(t *testing.T) {
test("-b --string foo test bar", kvs{"bool": true, "string": "foo"}, words{"bar"})
test("-b=false --string bar", kvs{"bool": false, "string": "bar"}, words{})
test("--strings a --strings b", kvs{"strings": []string{"a", "b"}}, words{})

test("--delimstrings a,b", kvs{"delimstrings": []string{"a", "b"}}, words{})
test("--delimstrings=a,b", kvs{"delimstrings": []string{"a", "b"}}, words{})
test("-d a,b", kvs{"delimstrings": []string{"a", "b"}}, words{})
test("-d=a,b", kvs{"delimstrings": []string{"a", "b"}}, words{})
test("-d=a,b -d c --delimstrings d", kvs{"delimstrings": []string{"a", "b", "c", "d"}}, words{})

testFail("foo test")
test("defaults", kvs{"opt": "def"}, words{})
test("defaults -o foo", kvs{"opt": "foo"}, words{})
Expand All @@ -170,6 +178,148 @@ func TestOptionParsing(t *testing.T) {
testFail("-zz--- --")
}

func TestDefaultOptionParsing(t *testing.T) {
testPanic := func(f func()) {
fnFinished := false
defer func() {
if r := recover(); fnFinished == true {
panic(r)
}
}()
f()
fnFinished = true
t.Error("expected panic")
}

testPanic(func() { cmds.StringOption("string", "s", "a string").WithDefault(0) })
testPanic(func() { cmds.StringOption("string", "s", "a string").WithDefault(false) })
testPanic(func() { cmds.StringOption("string", "s", "a string").WithDefault(nil) })
testPanic(func() { cmds.StringOption("string", "s", "a string").WithDefault([]string{"foo"}) })
testPanic(func() { cmds.StringsOption("strings", "a", "a string array").WithDefault(0) })
testPanic(func() { cmds.StringsOption("strings", "a", "a string array").WithDefault(false) })
testPanic(func() { cmds.StringsOption("strings", "a", "a string array").WithDefault(nil) })
testPanic(func() { cmds.StringsOption("strings", "a", "a string array").WithDefault("foo") })
testPanic(func() { cmds.StringsOption("strings", "a", "a string array").WithDefault([]bool{false}) })
testPanic(func() { cmds.DelimitedStringsOption(",", "dstrings", "d", "delimited string array").WithDefault(0) })
testPanic(func() { cmds.DelimitedStringsOption(",", "dstrs", "d", "delimited string array").WithDefault(false) })
testPanic(func() { cmds.DelimitedStringsOption(",", "dstrings", "d", "delimited string array").WithDefault(nil) })
testPanic(func() { cmds.DelimitedStringsOption(",", "dstrs", "d", "delimited string array").WithDefault("foo") })
testPanic(func() { cmds.DelimitedStringsOption(",", "dstrs", "d", "delimited string array").WithDefault([]int{0}) })

testPanic(func() { cmds.BoolOption("bool", "b", "a bool").WithDefault(0) })
testPanic(func() { cmds.BoolOption("bool", "b", "a bool").WithDefault(1) })
testPanic(func() { cmds.BoolOption("bool", "b", "a bool").WithDefault(nil) })
testPanic(func() { cmds.BoolOption("bool", "b", "a bool").WithDefault([]bool{false}) })
testPanic(func() { cmds.BoolOption("bool", "b", "a bool").WithDefault([]string{"foo"}) })

testPanic(func() { cmds.UintOption("uint", "u", "a uint").WithDefault(int(0)) })
testPanic(func() { cmds.UintOption("uint", "u", "a uint").WithDefault(int32(0)) })
testPanic(func() { cmds.UintOption("uint", "u", "a uint").WithDefault(int64(0)) })
testPanic(func() { cmds.UintOption("uint", "u", "a uint").WithDefault(uint64(0)) })
testPanic(func() { cmds.UintOption("uint", "u", "a uint").WithDefault(uint32(0)) })
testPanic(func() { cmds.UintOption("uint", "u", "a uint").WithDefault(float32(0)) })
testPanic(func() { cmds.UintOption("uint", "u", "a uint").WithDefault(float64(0)) })
testPanic(func() { cmds.UintOption("uint", "u", "a uint").WithDefault(nil) })
testPanic(func() { cmds.UintOption("uint", "u", "a uint").WithDefault([]uint{0}) })
testPanic(func() { cmds.UintOption("uint", "u", "a uint").WithDefault([]string{"foo"}) })
testPanic(func() { cmds.Uint64Option("uint64", "v", "a uint64").WithDefault(int(0)) })
testPanic(func() { cmds.Uint64Option("uint64", "v", "a uint64").WithDefault(int32(0)) })
testPanic(func() { cmds.Uint64Option("uint64", "v", "a uint64").WithDefault(int64(0)) })
testPanic(func() { cmds.Uint64Option("uint64", "v", "a uint64").WithDefault(uint(0)) })
testPanic(func() { cmds.Uint64Option("uint64", "v", "a uint64").WithDefault(uint32(0)) })
testPanic(func() { cmds.Uint64Option("uint64", "v", "a uint64").WithDefault(float32(0)) })
testPanic(func() { cmds.Uint64Option("uint64", "v", "a uint64").WithDefault(float64(0)) })
testPanic(func() { cmds.Uint64Option("uint64", "v", "a uint64").WithDefault(nil) })
testPanic(func() { cmds.Uint64Option("uint64", "v", "a uint64").WithDefault([]uint64{0}) })
testPanic(func() { cmds.Uint64Option("uint64", "v", "a uint64").WithDefault([]string{"foo"}) })
testPanic(func() { cmds.IntOption("int", "i", "an int").WithDefault(int32(0)) })
testPanic(func() { cmds.IntOption("int", "i", "an int").WithDefault(int64(0)) })
testPanic(func() { cmds.IntOption("int", "i", "an int").WithDefault(uint(0)) })
testPanic(func() { cmds.IntOption("int", "i", "an int").WithDefault(uint32(0)) })
testPanic(func() { cmds.IntOption("int", "i", "an int").WithDefault(uint64(0)) })
testPanic(func() { cmds.IntOption("int", "i", "an int").WithDefault(float32(0)) })
testPanic(func() { cmds.IntOption("int", "i", "an int").WithDefault(float64(0)) })
testPanic(func() { cmds.IntOption("int", "i", "an int").WithDefault(nil) })
testPanic(func() { cmds.IntOption("int", "i", "an int").WithDefault([]int{0}) })
testPanic(func() { cmds.IntOption("int", "i", "an int").WithDefault([]string{"foo"}) })
testPanic(func() { cmds.Int64Option("int64", "j", "an int64").WithDefault(int(0)) })
testPanic(func() { cmds.Int64Option("int64", "j", "an int64").WithDefault(int32(0)) })
testPanic(func() { cmds.Int64Option("int64", "j", "an int64").WithDefault(uint(0)) })
testPanic(func() { cmds.Int64Option("int64", "j", "an int64").WithDefault(uint32(0)) })
testPanic(func() { cmds.Int64Option("int64", "j", "an int64").WithDefault(uint64(0)) })
testPanic(func() { cmds.Int64Option("int64", "j", "an int64").WithDefault(float32(0)) })
testPanic(func() { cmds.Int64Option("int64", "j", "an int64").WithDefault(float64(0)) })
testPanic(func() { cmds.Int64Option("int64", "j", "an int64").WithDefault(nil) })
testPanic(func() { cmds.Int64Option("int64", "j", "an int64").WithDefault([]int64{0}) })
testPanic(func() { cmds.Int64Option("int64", "j", "an int64").WithDefault([]string{"foo"}) })
testPanic(func() { cmds.FloatOption("float", "f", "a float64").WithDefault(int(0)) })
testPanic(func() { cmds.FloatOption("float", "f", "a float64").WithDefault(int32(0)) })
testPanic(func() { cmds.FloatOption("float", "f", "a float64").WithDefault(int64(0)) })
testPanic(func() { cmds.FloatOption("float", "f", "a float64").WithDefault(uint(0)) })
testPanic(func() { cmds.FloatOption("float", "f", "a float64").WithDefault(uint32(0)) })
testPanic(func() { cmds.FloatOption("float", "f", "a float64").WithDefault(uint64(0)) })
testPanic(func() { cmds.FloatOption("float", "f", "a float64").WithDefault(float32(0)) })
testPanic(func() { cmds.FloatOption("float", "f", "a float64").WithDefault(nil) })
testPanic(func() { cmds.FloatOption("float", "f", "a float64").WithDefault([]int{0}) })
testPanic(func() { cmds.FloatOption("float", "f", "a float64").WithDefault([]string{"foo"}) })

cmd := &cmds.Command{
Subcommands: map[string]*cmds.Command{
"defaults": &cmds.Command{
Options: []cmds.Option{
cmds.StringOption("string", "s", "a string").WithDefault("foo"),
cmds.StringsOption("strings1", "a", "a string array").WithDefault([]string{"foo"}),
cmds.StringsOption("strings2", "b", "a string array").WithDefault([]string{"foo", "bar"}),
cmds.DelimitedStringsOption(",", "dstrings1", "c", "a delimited string array").WithDefault([]string{"foo"}),
cmds.DelimitedStringsOption(",", "dstrings2", "d", "a delimited string array").WithDefault([]string{"foo", "bar"}),

cmds.BoolOption("boolT", "t", "a bool").WithDefault(true),
cmds.BoolOption("boolF", "a bool").WithDefault(false),

cmds.UintOption("uint", "u", "a uint").WithDefault(uint(1)),
cmds.Uint64Option("uint64", "v", "a uint64").WithDefault(uint64(1)),
cmds.IntOption("int", "i", "an int").WithDefault(int(1)),
cmds.Int64Option("int64", "j", "an int64").WithDefault(int64(1)),
cmds.FloatOption("float", "f", "a float64").WithDefault(float64(1)),
},
},
},
}

test := func(args string, expectedOpts kvs, expectedWords words) {
testOptionHelper(t, cmd, args, expectedOpts, expectedWords, false)
}

test("defaults", kvs{
"string": "foo",
"strings1": []string{"foo"},
"strings2": []string{"foo", "bar"},
"dstrings1": []string{"foo"},
"dstrings2": []string{"foo", "bar"},
"boolT": true,
"boolF": false,
"uint": uint(1),
"uint64": uint64(1),
"int": int(1),
"int64": int64(1),
"float": float64(1),
}, words{})
test("defaults --string baz --strings1=baz -b baz -b=foo -c=foo -d=foo,baz,bing -d=zip,zap -d=zorp -t=false --boolF -u=0 -v=10 -i=-5 -j=10 -f=-3.14", kvs{
"string": "baz",
"strings1": []string{"baz"},
"strings2": []string{"baz", "foo"},
"dstrings1": []string{"foo"},
"dstrings2": []string{"foo", "baz", "bing", "zip", "zap", "zorp"},
"boolT": false,
"boolF": true,
"uint": uint(0),
"uint64": uint64(10),
"int": int(-5),
"int64": int64(10),
"float": float64(-3.14),
}, words{})
}

func TestArgumentParsing(t *testing.T) {
rootCmd := &cmds.Command{
Subcommands: map[string]*cmds.Command{
Expand Down
52 changes: 51 additions & 1 deletion option.go
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,18 @@ func NewOption(kind reflect.Kind, names ...string) Option {
}

func (o *option) WithDefault(v interface{}) Option {
if v == nil {
panic(fmt.Errorf("cannot use nil as a default"))
}
Comment on lines +152 to +154
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not seeing any problem with this ATM, it seems like so far all of our types are concrete and even for the pointer types we do have (i.e. []string) as long as people pass in var emptyStingrArray []string instead of just the untyped nil it will be fine.


// if type of value does not match the option type
if vKind, oKind := reflect.TypeOf(v).Kind(), o.Type(); vKind != oKind {
aschmahmann marked this conversation as resolved.
Show resolved Hide resolved
// if the reason they do not match is not because of Slice vs Array equivalence
// Note: Figuring out if the type of Slice/Array matches is not done in this function
if !((vKind == reflect.Array || vKind == reflect.Slice) && (oKind == reflect.Array || oKind == reflect.Slice)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be removed, but basically is just a hack for StringsOption so that it can still opaquely call this function and everything works. Doesn't seem too unreasonable though.

panic(fmt.Errorf("invalid default for the given type, expected %s got %s", o.Type(), vKind))
}
}
o.defaultVal = v
return o
}
Expand Down Expand Up @@ -184,6 +196,44 @@ func FloatOption(names ...string) Option {
func StringOption(names ...string) Option {
return NewOption(String, names...)
}

// StringsOption is a command option that can handle a slice of strings
func StringsOption(names ...string) Option {
return NewOption(Strings, names...)
return DelimitedStringsOption("", names...)
}

// DelimitedStringsOption like StringsOption is a command option that can handle a slice of strings.
// However, DelimitedStringsOption will automatically break up the associated CLI inputs based on the delimiter.
// For example, instead of passing `command --option=val1 --option=val2` you can pass `command --option=val1,val2` or
// even `command --option=val1,val2 --option=val3,val4`.
//
// A delimiter of "" means that no delimiter is used
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is as opposed to strings.Split where an empty delimiter means to split on every character. We could add this behavior, but I'm not currently seeing the utility.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is best to avoid splitting on every character and insist on a delimiter to separate items in an option string.

You could even go one step farther, and not allow an empty delimiter to be passed to DelimitedStringsOption, thereby forcing the use of StringsOption for options that cannot be delimited. That would eliminate any ambiguity of specifying "" as the delimiter, as well as prevent having two ways to create an undelimited string option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gammazero @lidel I pushed an update here that panics when an empty delimiter is passed, but still uses the same underlying struct for StringsOption and DelimitedStringsOption with StringsOption using an empty delimiter. let me know what you think and we'll merge 🚢

func DelimitedStringsOption(delimiter string, names ...string) Option {
aschmahmann marked this conversation as resolved.
Show resolved Hide resolved
return &stringsOption{
Option: NewOption(Strings, names...),
delimiter: delimiter,
}
}

type stringsOption struct {
Option
delimiter string
}

func (s *stringsOption) WithDefault(v interface{}) Option {
if v == nil {
return s.Option.WithDefault(v)
}

defVal := v.([]string)
s.Option = s.Option.WithDefault(defVal)
return s
}

func (s *stringsOption) Parse(v string) (interface{}, error) {
if s.delimiter == "" {
return []string{v}, nil
}

return strings.Split(v, s.delimiter), nil
}