Skip to content
This repository was archived by the owner on Aug 23, 2023. It is now read-only.

Accept bool strings for ArgBool #895

Merged
merged 3 commits into from
Apr 30, 2018
Merged

Accept bool strings for ArgBool #895

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ea5a765
Support conversion of string to bool
shanson7 Apr 20, 2018
402189a
Merge pull request #20 from bloomberg/strBool
yogeswaran Apr 20, 2018
954fd2b
use idiomatic name 'ok' for if
shanson7 Apr 30, 2018
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
26 changes: 20 additions & 6 deletions expr/expr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,10 +149,17 @@ func (e expr) consumeBasicArg(pos int, exp Arg) (int, error) {
}
*v.val = re
case ArgBool:
if got.etype != etBool {
return 0, ErrBadArgumentStr{"string", got.etype.String()}
if got.etype == etBool {
*v.val = got.bool
break
}
if got.etype == etString {
if val, ok := strToBool(got.str); ok {
*v.val = val
break
}
}
*v.val = got.bool
return 0, ErrBadArgumentStr{"boolean", got.etype.String()}
case ArgStringsOrInts:
// consume all args (if any) in args that will yield a string or int
for ; len(e.args) > pos && (e.args[pos].etype == etString || e.args[pos].etype == etInt); pos++ {
Expand Down Expand Up @@ -271,10 +278,17 @@ func (e expr) consumeKwarg(key string, optArgs []Arg) error {
}
*v.val = got.str
case ArgBool:
if got.etype != etBool {
return ErrBadKwarg{key, exp, got.etype}
if got.etype == etBool {
*v.val = got.bool
break
}
if got.etype == etString {
if val, ok := strToBool(got.str); ok {
*v.val = val
break
}
}
*v.val = got.bool
return ErrBadKwarg{key, exp, got.etype}
default:
return fmt.Errorf("unsupported type %T for consumeKwarg", exp)
}
Expand Down
25 changes: 19 additions & 6 deletions expr/parse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,12 +112,13 @@ func Parse(e string) (*expr, string, error) {
return parseConst(e)
}

if strings.HasPrefix(e, "True") || strings.HasPrefix(e, "true") {
return &expr{bool: true, str: e[:4], etype: etBool}, e[4:], nil
}

if strings.HasPrefix(e, "False") || strings.HasPrefix(e, "false") {
return &expr{bool: false, str: e[:5], etype: etBool}, e[5:], nil
if val, ok := strToBool(e); ok {
// 'false' is 5 chars, 'true' is 4
size := 5
if val {
size = 4
}
return &expr{bool: val, str: e[:size], etype: etBool}, e[size:], nil
Copy link
Contributor

Choose a reason for hiding this comment

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

why is etype etBool here and not etString?

Copy link
Collaborator Author

@shanson7 shanson7 Apr 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This case is an unquoted boolean (e.g. false not 'false' or "false"). This makes it unambiguously an etBool

Copy link
Contributor

Choose a reason for hiding this comment

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

ah you got me. You know this code better now than I do 😄

}

if e[0] == '\'' || e[0] == '"' {
Expand Down Expand Up @@ -151,6 +152,18 @@ func Parse(e string) (*expr, string, error) {
return &expr{str: name, etype: etName}, e, nil
}

func strToBool(val string) (bool, bool) {
if strings.HasPrefix(val, "True") || strings.HasPrefix(val, "true") {
return true, true
}

if strings.HasPrefix(val, "False") || strings.HasPrefix(val, "false") {
return false, true
}

return false, false
}

// caller must assure s starts with opening paren
func parseArgList(e string) (string, []*expr, map[string]*expr, string, error) {

Expand Down
16 changes: 16 additions & 0 deletions expr/parse_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,22 @@ func TestParse(t *testing.T) {
},
nil,
},
{
"func(metric, key2='true', key1='false')",
&expr{
str: "func",
etype: etFunc,
args: []*expr{
{str: "metric"},
},
namedArgs: map[string]*expr{
"key2": {etype: etString, str: "true"},
"key1": {etype: etString, str: "false"},
Copy link
Contributor

Choose a reason for hiding this comment

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

don't we want these to be etBool ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense

},
argsStr: "metric, key2='true', key1='false'",
},
nil,
},

{
`foo.{bar,baz}.qux`,
Expand Down
29 changes: 29 additions & 0 deletions expr/plan_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,20 @@ func TestArgs(t *testing.T) {
},
nil,
},
{
"2 args normal, 2 optional by position (bools as strings)",
[]*expr{
{etype: etName, str: "foo.bar.*"},
{etype: etString, str: "1hour"},
{etype: etString, str: "sum"},
{etype: etString, str: "false"},
},
nil,
[]Req{
NewReq("foo.bar.*", from, to, 0),
},
nil,
},
{
"2 args normal, 2 optional by key",
[]*expr{
Expand All @@ -63,6 +77,21 @@ func TestArgs(t *testing.T) {
},
nil,
},
{
"2 args normal, 2 optional by key (bools as strings)",
[]*expr{
{etype: etName, str: "foo.bar.*"},
{etype: etString, str: "1hour"},
},
map[string]*expr{
"func": {etype: etString, str: "sum"},
"alignToFrom": {etype: etString, str: "true"},
},
[]Req{
NewReq("foo.bar.*", from, to, 0),
},
nil,
},
{
"2 args normal, 1 by position, 1 by keyword",
[]*expr{
Expand Down