StringsOption with delimiter #204
Conversation
aschmahmann
force-pushed
the
feat/stringsDelim
branch
2 times, most recently
from
9aabff1
to
dcd501d
Compare
…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
… those Options using the CLI parsing tools
aschmahmann
force-pushed
the
feat/stringsDelim
branch
from
dcd501d
to
e2c3e22
Compare
option.go
Outdated
| // 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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 🚢
| if v == nil { | ||
| panic(fmt.Errorf("cannot use nil as a default")) | ||
| } |
There was a problem hiding this comment.
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.
| @@ -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) { | |||
There was a problem hiding this comment.
this was just extracted from the function below
| 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)) { |
There was a problem hiding this comment.
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.
option.go
Outdated
| // 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 |
There was a problem hiding this comment.
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.
Added a variant of StringsOption that supports getting delimited lists on the CLI for conciseness.
Also, made
WithDefaultpanic when passing in the wrong type.Note: Delimited only works on the CLI. This means we should probably return an error if the HTTP API tries to pass a string with a delimiter in it, but that's not done yet. Callers should handle bad strings so this doesn't seem particularly problematic.