From 382fd11f8d0aa47ade40d5a9d4c89e5c1fcce06f Mon Sep 17 00:00:00 2001 From: Adin Schmahmann Date: Thu, 14 Jan 2021 17:01:11 -0500 Subject: [PATCH 1/6] feat: StringsOption.Parse(string) now returns a slice containing the original string. Added DelimitedStringsOption which allows passing delimited options on the CLI, such as --option=opt1,opt2 when the delimiter is a comma, instead of requiring the longer form --option=op1 --option=opt2 --- cli/parse.go | 2 +- option.go | 40 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/cli/parse.go b/cli/parse.go index ae74d987..eebd6b43 100644 --- a/cli/parse.go +++ b/cli/parse.go @@ -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 { diff --git a/option.go b/option.go index 68962bc4..21b59b75 100644 --- a/option.go +++ b/option.go @@ -184,6 +184,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 +func DelimitedStringsOption(delimiter string, names ...string) Option { + 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 } From cde56b1d1f1b94179a1c6e954067eeae2a1698b8 Mon Sep 17 00:00:00 2001 From: Adin Schmahmann Date: Fri, 15 Jan 2021 00:09:44 -0500 Subject: [PATCH 2/6] testing: added DelimitedStringsOption parsing tests --- cli/parse_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/cli/parse_test.go b/cli/parse_test.go index bc6e2aea..db8d0cab 100644 --- a/cli/parse_test.go +++ b/cli/parse_test.go @@ -84,6 +84,7 @@ func TestOptionParsing(t *testing.T) { 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{}, @@ -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{}) From d48b43da090ac027b20ed2f4d344de25feb66973 Mon Sep 17 00:00:00 2001 From: Adin Schmahmann Date: Fri, 15 Jan 2021 04:10:15 -0500 Subject: [PATCH 3/6] testing: refactor to extract testOptionHelper for reuse --- cli/parse_test.go | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/cli/parse_test.go b/cli/parse_test.go index db8d0cab..33d8aebf 100644 --- a/cli/parse_test.go +++ b/cli/parse_test.go @@ -77,6 +77,24 @@ 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) { + 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{ @@ -96,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{"-"}) From 16d8195b08175a3d0e9eb8e62deb2b35b62656bc Mon Sep 17 00:00:00 2001 From: Adin Schmahmann Date: Fri, 15 Jan 2021 01:01:35 -0500 Subject: [PATCH 4/6] feat: calling WithDefaults with the wrong type now panics --- option.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/option.go b/option.go index 21b59b75..3247e577 100644 --- a/option.go +++ b/option.go @@ -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")) + } + + // if type of value does not match the option type + if vKind, oKind := reflect.TypeOf(v).Kind(), o.Type(); vKind != oKind { + // 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)) { + panic(fmt.Errorf("invalid default for the given type, expected %s got %s", o.Type(), vKind)) + } + } o.defaultVal = v return o } From e2c3e22d578c1026e54857aa6f8166e6c73d4e46 Mon Sep 17 00:00:00 2001 From: Adin Schmahmann Date: Fri, 15 Jan 2021 03:55:31 -0500 Subject: [PATCH 5/6] testing: add tests for constructing Options with defaults and parsing those Options using the CLI parsing tools --- cli/parse_test.go | 142 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 142 insertions(+) diff --git a/cli/parse_test.go b/cli/parse_test.go index 33d8aebf..5fd52198 100644 --- a/cli/parse_test.go +++ b/cli/parse_test.go @@ -178,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{ From c6690cc1e7676003f9c66cbba986a13e096d9f9f Mon Sep 17 00:00:00 2001 From: Adin Schmahmann Date: Tue, 26 Jan 2021 13:33:34 -0500 Subject: [PATCH 6/6] feat: DelimitedStringsOption now panics if an empty delimiter is passed in --- option.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/option.go b/option.go index 3247e577..3b69c3c4 100644 --- a/option.go +++ b/option.go @@ -199,7 +199,10 @@ func StringOption(names ...string) Option { // StringsOption is a command option that can handle a slice of strings func StringsOption(names ...string) Option { - return DelimitedStringsOption("", names...) + return &stringsOption{ + Option: NewOption(Strings, names...), + delimiter: "", + } } // DelimitedStringsOption like StringsOption is a command option that can handle a slice of strings. @@ -207,8 +210,11 @@ func StringsOption(names ...string) Option { // 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 +// A delimiter of "" is invalid func DelimitedStringsOption(delimiter string, names ...string) Option { + if delimiter == "" { + panic("cannot create a DelimitedStringsOption with no delimiter") + } return &stringsOption{ Option: NewOption(Strings, names...), delimiter: delimiter,