Accept bool strings for ArgBool #895
Conversation
Automatic conversion of bool strings to bools
expr/parse.go
Outdated
|
|
||
| if strings.HasPrefix(e, "False") || strings.HasPrefix(e, "false") { | ||
| return &expr{bool: false, str: e[:5], etype: etBool}, e[5:], nil | ||
| if val, wasBool := strToBool(e); wasBool { |
There was a problem hiding this comment.
i'm confused about how we support strings supplied by the user that have quotes.
why are we not comparing e to "\"False\"" anywhere for example?
There was a problem hiding this comment.
I chose not to interpret all strings like 'false' and "True" etc. as bools automatically, because things like alias could very well be passed 'false' as a string value and intend for it to be a string.
Instead, I changed the ArgBool cases in expr.go to also accept strings that look like a bool
expr/expr.go
Outdated
| break | ||
| } | ||
| if got.etype == etString { | ||
| if val, wasBool := strToBool(got.str); wasBool { |
There was a problem hiding this comment.
golang idiom to use ok, not wasBool
| }, | ||
| namedArgs: map[string]*expr{ | ||
| "key2": {etype: etString, str: "true"}, | ||
| "key1": {etype: etString, str: "false"}, |
There was a problem hiding this comment.
don't we want these to be etBool ?
There was a problem hiding this comment.
At parse time, we don't know what type we want. We only know what type they are. Only when they are converted to expressions do we know what signature to apply and how to match them up.
| if val { | ||
| size = 4 | ||
| } | ||
| return &expr{bool: val, str: e[:size], etype: etBool}, e[size:], nil |
There was a problem hiding this comment.
why is etype etBool here and not etString?
There was a problem hiding this comment.
This case is an unquoted boolean (e.g. false not 'false' or "false"). This makes it unambiguously an etBool
There was a problem hiding this comment.
ah you got me. You know this code better now than I do 😄
expr/expr.go
Outdated
| break | ||
| } | ||
| if got.etype == etString { | ||
| if val, wasBool := strToBool(got.str); wasBool { |
There was a problem hiding this comment.
how about we make strToBool a method on expr ? we just comment that it's only defined for etString types.
There was a problem hiding this comment.
We use the same logic in parse.go on the string e.
Fixes #867 (for string case)