diff --git a/policy/predicate/file.go b/policy/predicate/file.go index 918ab6de..e832692b 100644 --- a/policy/predicate/file.go +++ b/policy/predicate/file.go @@ -15,9 +15,9 @@ package predicate import ( + "bytes" "context" "fmt" - "regexp" "strconv" "github.com/palantir/policy-bot/policy/common" @@ -96,35 +96,80 @@ type ModifiedLines struct { Total ComparisonExpr `yaml:"total"` } -type ComparisonExpr string +type CompareOp uint8 -var ( - numCompRegexp = regexp.MustCompile(`^(<|>) ?(\d+)$`) +const ( + OpNone CompareOp = iota + OpLessThan + OpGreaterThan ) +type ComparisonExpr struct { + Op CompareOp + Value int64 +} + func (exp ComparisonExpr) IsEmpty() bool { - return exp == "" + return exp.Op == OpNone && exp.Value == 0 } -func (exp ComparisonExpr) Evaluate(n int64) (bool, error) { - match := numCompRegexp.FindStringSubmatch(string(exp)) - if match == nil { - return false, errors.Errorf("invalid comparison expression: %q", exp) +func (exp ComparisonExpr) Evaluate(n int64) bool { + switch exp.Op { + case OpLessThan: + return n < exp.Value + case OpGreaterThan: + return n > exp.Value } + return false +} - op := match[1] - v, err := strconv.ParseInt(match[2], 10, 64) - if err != nil { - return false, errors.Wrapf(err, "invalid commparison expression: %q", exp) +func (exp ComparisonExpr) MarshalText() ([]byte, error) { + if exp.Op == OpNone { + return nil, nil } - switch op { - case "<": - return n < v, nil - case ">": - return n > v, nil + var op string + switch exp.Op { + case OpLessThan: + op = "<" + case OpGreaterThan: + op = ">" + default: + return nil, errors.Errorf("unknown operation: %d", exp.Op) } - return false, errors.Errorf("invalid comparison expression: %q", exp) + return []byte(fmt.Sprintf("%s %d", op, exp.Value)), nil +} + +func (exp *ComparisonExpr) UnmarshalText(text []byte) error { + text = bytes.TrimSpace(text) + if len(text) == 0 { + *exp = ComparisonExpr{} + return nil + } + + i := 0 + var op CompareOp + switch text[i] { + case '<': + op = OpLessThan + case '>': + op = OpGreaterThan + default: + return errors.Errorf("invalid comparison operator: %c", text[i]) + } + + i++ + for i < len(text) && (text[i] == ' ' || text[i] == '\t') { + i++ + } + + v, err := strconv.ParseInt(string(text[i:]), 10, 64) + if err != nil { + return errors.Wrapf(err, "invalid comparison value") + } + + *exp = ComparisonExpr{Op: op, Value: v} + return nil } func (pred *ModifiedLines) Evaluate(ctx context.Context, prctx pull.Context) (bool, string, error) { @@ -144,14 +189,8 @@ func (pred *ModifiedLines) Evaluate(ctx context.Context, prctx pull.Context) (bo pred.Deletions: deletions, pred.Total: additions + deletions, } { - if !expr.IsEmpty() { - ok, err := expr.Evaluate(v) - if err != nil { - return false, "", err - } - if ok { - return true, "", nil - } + if !expr.IsEmpty() && expr.Evaluate(v) { + return true, "", nil } } return false, fmt.Sprintf("modification of (+%d, -%d) does not match any conditions", additions, deletions), nil diff --git a/policy/predicate/file_test.go b/policy/predicate/file_test.go index 73fd0ab3..6a8477e2 100644 --- a/policy/predicate/file_test.go +++ b/policy/predicate/file_test.go @@ -176,8 +176,8 @@ func TestOnlyChangedFiles(t *testing.T) { func TestModifiedLines(t *testing.T) { p := &ModifiedLines{ - Additions: ">100", - Deletions: ">10", + Additions: ComparisonExpr{Op: OpGreaterThan, Value: 100}, + Deletions: ComparisonExpr{Op: OpGreaterThan, Value: 10}, } runFileTests(t, p, []FileTestCase{ @@ -208,7 +208,7 @@ func TestModifiedLines(t *testing.T) { }) p = &ModifiedLines{ - Total: ">100", + Total: ComparisonExpr{Op: OpGreaterThan, Value: 100}, } runFileTests(t, p, []FileTestCase{ @@ -230,55 +230,89 @@ func TestComparisonExpr(t *testing.T) { Expr ComparisonExpr Value int64 Output bool - Err bool }{ "greaterThanTrue": { - Expr: ">100", + Expr: ComparisonExpr{Op: OpGreaterThan, Value: 100}, Value: 200, Output: true, }, "greaterThanFalse": { - Expr: "> 100", + Expr: ComparisonExpr{Op: OpGreaterThan, Value: 100}, Value: 50, Output: false, }, "lessThanTrue": { - Expr: "<100", + Expr: ComparisonExpr{Op: OpLessThan, Value: 100}, Value: 50, Output: true, }, "lessThanFalse": { - Expr: "< 100", + Expr: ComparisonExpr{Op: OpLessThan, Value: 100}, Value: 200, Output: false, }, - "invalidOperator": { - Expr: "=200", - Err: true, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + ok := test.Expr.Evaluate(test.Value) + assert.Equal(t, test.Output, ok, "evaluation was not correct") + }) + } + + t.Run("isEmpty", func(t *testing.T) { + assert.True(t, ComparisonExpr{}.IsEmpty(), "expression was not empty") + assert.False(t, ComparisonExpr{Op: OpGreaterThan, Value: 100}.IsEmpty(), "expression was empty") + }) + + parseTests := map[string]struct { + Input string + Output ComparisonExpr + Err bool + }{ + "lessThan": { + Input: "<100", + Output: ComparisonExpr{Op: OpLessThan, Value: 100}, + }, + "greaterThan": { + Input: ">100", + Output: ComparisonExpr{Op: OpGreaterThan, Value: 100}, + }, + "innerSpaces": { + Input: "< 35", + Output: ComparisonExpr{Op: OpLessThan, Value: 35}, + }, + "leadingSpaces": { + Input: " < 35", + Output: ComparisonExpr{Op: OpLessThan, Value: 35}, + }, + "trailngSpaces": { + Input: "< 35 ", + Output: ComparisonExpr{Op: OpLessThan, Value: 35}, }, - "invalidNumber": { - Expr: ">12ab", - Err: true, + "invalidOp": { + Input: "=10", + Err: true, + }, + "invalidValue": { + Input: "< 10ab", + Err: true, }, } - for name, test := range tests { + for name, test := range parseTests { t.Run(name, func(t *testing.T) { - ok, err := test.Expr.Evaluate(test.Value) + var expr ComparisonExpr + err := expr.UnmarshalText([]byte(test.Input)) if test.Err { - assert.Error(t, err, "expected error evaluating expression, but got nil") + assert.Error(t, err, "expected error parsing expression, but got nil") return } - if assert.NoError(t, err, "unexpected error evaluating expression") { - assert.Equal(t, test.Output, ok, "evaluation was not correct") + if assert.NoError(t, err, "unexpected error parsing expression") { + assert.Equal(t, test.Output, expr, "parsed expression was not correct") } }) } - - t.Run("isEmpty", func(t *testing.T) { - assert.True(t, ComparisonExpr("").IsEmpty(), "expression was not empty") - assert.False(t, ComparisonExpr(">100").IsEmpty(), "expression was empty") - }) } type FileTestCase struct {