From 604a4bd947dd3c724ca634f99d1e7a363fea1e74 Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Tue, 28 Jun 2022 19:01:19 +0000 Subject: [PATCH 1/7] Support formatting options when unparsing ASTs to strings (#293) This change adds formatting capability to unparser by introducing a new function named `UnparseFormatted`. The function will accept any number of `UnparserFormattingOption` to support adding new lines for any desired non-unary operators. --- common/operators/operators.go | 10 ++ parser/unparser.go | 114 +++++++++++++++-- parser/unparser_test.go | 230 +++++++++++++++++++++++++++++++++- 3 files changed, 342 insertions(+), 12 deletions(-) diff --git a/common/operators/operators.go b/common/operators/operators.go index 8a2e9094..fa25dfb7 100644 --- a/common/operators/operators.go +++ b/common/operators/operators.go @@ -141,3 +141,13 @@ func Precedence(symbol string) int { } return op.precedence } + +// Arity returns the number of argument the operator takes +// -1 is returned if an undefined symbol is provided +func Arity(symbol string) int { + op, found := operatorMap[symbol] + if !found { + return -1 + } + return op.arity +} diff --git a/parser/unparser.go b/parser/unparser.go index 6a610ff7..ece704c8 100644 --- a/parser/unparser.go +++ b/parser/unparser.go @@ -36,9 +36,26 @@ import ( // - Floating point values are converted to the small number of digits needed to represent the value. // - Spacing around punctuation marks may be lost. // - Parentheses will only be applied when they affect operator precedence. -func Unparse(expr *exprpb.Expr, info *exprpb.SourceInfo) (string, error) { - un := &unparser{info: info} - err := un.visit(expr) +// +// This function optionally takes in one or more UnparserFormattingOption to dictate the formatting of the output. +func Unparse(expr *exprpb.Expr, info *exprpb.SourceInfo, opts ...UnparserFormattingOption) (string, error) { + formattingOpts := &formattingOption{ + wrapColumn: defaultWrapColumn, + } + + var err error + for _, opt := range opts { + formattingOpts, err = opt(formattingOpts) + if err != nil { + return "", err + } + } + + un := &unparser{ + info: info, + options: formattingOpts, + } + err = un.visit(expr) if err != nil { return "", err } @@ -47,8 +64,10 @@ func Unparse(expr *exprpb.Expr, info *exprpb.SourceInfo) (string, error) { // unparser visits an expression to reconstruct a human-readable string from an AST. type unparser struct { - str strings.Builder - info *exprpb.SourceInfo + str strings.Builder + info *exprpb.SourceInfo + options *formattingOption + lastWrappedIndex int } func (un *unparser) visit(expr *exprpb.Expr) error { @@ -135,9 +154,12 @@ func (un *unparser) visitCallBinary(expr *exprpb.Expr) error { if !found { return fmt.Errorf("cannot unmangle operator: %s", fun) } + un.str.WriteString(" ") un.str.WriteString(unmangled) - un.str.WriteString(" ") + if !un.wrapForOperator(fun) { + un.str.WriteString(" ") + } return un.visitMaybeNested(rhs, rhsParen) } @@ -151,7 +173,11 @@ func (un *unparser) visitCallConditional(expr *exprpb.Expr) error { if err != nil { return err } - un.str.WriteString(" ? ") + un.str.WriteString(" ?") + if !un.wrapForOperator(operators.Conditional) { + un.str.WriteString(" ") + } + // add parens if operand is a conditional itself. nested = isSamePrecedence(operators.Conditional, args[1]) || isComplexOperator(args[1]) @@ -159,6 +185,7 @@ func (un *unparser) visitCallConditional(expr *exprpb.Expr) error { if err != nil { return err } + un.str.WriteString(" : ") // add parens if operand is a conditional itself. nested = isSamePrecedence(operators.Conditional, args[2]) || @@ -444,3 +471,76 @@ func bytesToOctets(byteVal []byte) string { } return b.String() } + +// Takes in a function then inserts a newline based on the rules provided in formatting options +func (un *unparser) wrapForOperator(fun string) bool { + _, wrapOperatorExists := un.options.operatorsToWrapOn[fun] + lineLength := un.str.Len() - un.lastWrappedIndex + if wrapOperatorExists && lineLength >= un.options.wrapColumn { + un.lastWrappedIndex = un.str.Len() + un.str.WriteString("\n") + return true + } + return false +} + +// Defined defaults for the formatting options +var ( + defaultWrapColumn = 80 +) + +// UnparserFormattingOption is a funcitonal option for configuring the output formatting +// of the Unparse function. +type UnparserFormattingOption func(*formattingOption) (*formattingOption, error) + +// Internal representation of the UnparserFormattingOption type +type formattingOption struct { + wrapColumn int // Defaults to 80 + operatorsToWrapOn map[string]bool +} + +// WrapOnColumn returns an UnparserFormattingOption with the column limit set. +// Word wrapping will be performed when a line's length exceeds the limit +// for operators set by WrapOnOperators function. +// +// Example usage: +// +// Unparse(expr, sourceInfo, WrapColumn(3), WrapOnOperators(Operators.LogicalAnd)) +// +// This will insert a newline immediately after the logical AND operator for the below example input: +// +// Input: a && b +// Output: a &&\nb +func WrapColumn(col int) UnparserFormattingOption { + return func(opt *formattingOption) (*formattingOption, error) { + if col < 1 { + return nil, fmt.Errorf("Invalid formatting option. Wrap column value must be greater than or equal to 1. Got %v instead", col) + } + opt.wrapColumn = col + return opt, nil + } +} + +// WrapOnOperators returns an UnparserFormattedOption with operators to perform word wrapping on. +// Word wrapping is supported on non-unary symbolic operators. Refer to operators.go for the full list +// +// This will replace any previously supplied operators instead of merging them. +func WrapOnOperators(symbols ...string) UnparserFormattingOption { + return func(fmtOpt *formattingOption) (*formattingOption, error) { + fmtOpt.operatorsToWrapOn = make(map[string]bool) + for _, symbol := range symbols { + _, found := operators.FindReverse(symbol) + if !found { + return nil, fmt.Errorf("Invalid formatting option. Unsupported operator: %s", symbol) + } + arity := operators.Arity(symbol) + if arity < 2 { + return nil, fmt.Errorf("Invalid formatting option. Unary operators are unsupported: %s", symbol) + } + + fmtOpt.operatorsToWrapOn[symbol] = true + } + + return fmtOpt, nil + } +} diff --git a/parser/unparser_test.go b/parser/unparser_test.go index 7bb41d39..a20521cb 100644 --- a/parser/unparser_test.go +++ b/parser/unparser_test.go @@ -20,6 +20,7 @@ import ( "testing" "github.com/google/cel-go/common" + "github.com/google/cel-go/common/operators" "google.golang.org/protobuf/proto" exprpb "google.golang.org/genproto/googleapis/api/expr/v1alpha1" @@ -31,6 +32,7 @@ func TestUnparse(t *testing.T) { in string out interface{} requiresMacroCalls bool + formattingOptions []UnparserFormattingOption }{ {name: "call_add", in: `a + b - c`}, {name: "call_and", in: `a && b && c && d && e`}, @@ -138,6 +140,191 @@ func TestUnparse(t *testing.T) { in: `[1, 2, 3].map(x, x >= 2, x * 4).filter(x, x <= 10)`, requiresMacroCalls: true, }, + + // These expressions will not be wrapped because they haven't met the + // conditions required by the provided formatting options + { + name: "call_no_wrap_no_operators", + in: "a + b + c + d", + out: "a + b + c + d", + formattingOptions: []UnparserFormattingOption{ + WrapColumn(3), + }, + }, + { + name: "call_no_wrap_short_line", + in: "a + b + c + d", + out: "a + b + c + d", + formattingOptions: []UnparserFormattingOption{ + WrapColumn(12), + WrapOnOperators(operators.Add), + }, + }, + + // These expressions will be formatted based on the formatting options provided + { + name: "call_wrap_add", + in: "a + b - d * e", + out: "a +\nb - d * e", + formattingOptions: []UnparserFormattingOption{ + WrapColumn(3), + WrapOnOperators(operators.Add), + }, + }, + { + name: "call_wrap_add_and_subtract", + in: "a * b + c - d * e", + out: "a * b +\nc -\nd * e", + formattingOptions: []UnparserFormattingOption{ + WrapColumn(3), + WrapOnOperators(operators.Add, operators.Subtract), + }, + }, + { + name: "call_wrap_and", + in: "a && b && c && d && e", + out: "a &&\nb &&\nc &&\nd &&\ne", + formattingOptions: []UnparserFormattingOption{ + WrapColumn(3), + WrapOnOperators(operators.LogicalAnd), + }, + }, + { + name: "call_wrap_and_2", + in: "a && b", + out: "a &&\nb", + formattingOptions: []UnparserFormattingOption{ + WrapColumn(3), + WrapOnOperators(operators.LogicalAnd), + }, + }, + { + name: "call_wrap_conditional", + in: "a ? b : c ? d : e", + out: "a ?\nb : (c ?\nd : e)", + formattingOptions: []UnparserFormattingOption{ + WrapColumn(3), + WrapOnOperators(operators.Conditional), + }, + }, + { + name: "call_wrap_or", + in: "a || b || c || d || e", + out: "a ||\nb ||\nc ||\nd ||\ne", + formattingOptions: []UnparserFormattingOption{ + WrapColumn(3), + WrapOnOperators(operators.LogicalOr), + }, + }, + { + name: "call_wrap_equals", + in: "a == b == c == d == e", + out: "a ==\nb ==\nc ==\nd ==\ne", + formattingOptions: []UnparserFormattingOption{ + WrapColumn(3), + WrapOnOperators(operators.Equals), + }, + }, + { + name: "call_wrap_greater", + in: "a > b > c > d > e", + out: "a >\nb >\nc >\nd >\ne", + formattingOptions: []UnparserFormattingOption{ + WrapColumn(3), + WrapOnOperators(operators.Greater), + }, + }, + { + name: "call_wrap_greater_equals", + in: "a >= b >= c >= d >= e", + out: "a >=\nb >=\nc >=\nd >=\ne", + formattingOptions: []UnparserFormattingOption{ + WrapColumn(3), + WrapOnOperators(operators.GreaterEquals), + }, + }, + { + name: "call_wrap_in", + in: "a in b in c in d in e", + out: "a in\nb in\nc in\nd in\ne", + formattingOptions: []UnparserFormattingOption{ + WrapColumn(3), + WrapOnOperators(operators.In), + }, + }, + { + name: "call_wrap_less", + in: "a < b < c < d < e", + out: "a <\nb <\nc <\nd <\ne", + formattingOptions: []UnparserFormattingOption{ + WrapColumn(3), + WrapOnOperators(operators.Less), + }, + }, + { + name: "call_wrap_less_equals", + in: "a <= b <= c <= d <= e", + out: "a <=\nb <=\nc <=\nd <=\ne", + formattingOptions: []UnparserFormattingOption{ + WrapColumn(3), + WrapOnOperators(operators.LessEquals), + }, + }, + { + name: "call_wrap_not_equals", + in: "a != b != c != d != e", + out: "a !=\nb !=\nc !=\nd !=\ne", + formattingOptions: []UnparserFormattingOption{ + WrapColumn(3), + WrapOnOperators(operators.NotEquals), + }, + }, + { + name: "call_wrap_divide", + in: "a / b / c / d / e", + out: "a /\nb /\nc /\nd /\ne", + formattingOptions: []UnparserFormattingOption{ + WrapColumn(3), + WrapOnOperators(operators.Divide), + }, + }, + { + name: "call_wrap_modulo", + in: "a % b % c % d % e", + out: "a %\nb %\nc %\nd %\ne", + formattingOptions: []UnparserFormattingOption{ + WrapColumn(3), + WrapOnOperators(operators.Modulo), + }, + }, + { + name: "call_wrap_multiply", + in: "a * b * c * d * e", + out: "a *\nb *\nc *\nd *\ne", + formattingOptions: []UnparserFormattingOption{ + WrapColumn(3), + WrapOnOperators(operators.Multiply), + }, + }, + { + name: "call_wrap_and_long_variables", + in: "longVariableA && longVariableB && longVariableC", + out: "longVariableA &&\nlongVariableB &&\nlongVariableC", + formattingOptions: []UnparserFormattingOption{ + WrapColumn(3), + WrapOnOperators(operators.LogicalAnd), + }, + }, + { + name: "comp_chained_wrap_comparisons", + in: "[1, 2, 3].map(x, x >= 2, x * 4).filter(x, x <= 10)", + out: "[1, 2, 3].map(x, x >=\n2, x * 4).filter(x, x <=\n10)", + requiresMacroCalls: true, + formattingOptions: []UnparserFormattingOption{ + WrapColumn(3), + WrapOnOperators(operators.GreaterEquals, operators.LessEquals), + }, + }, } for _, tst := range tests { @@ -154,7 +341,8 @@ func TestUnparse(t *testing.T) { if len(iss.GetErrors()) > 0 { t.Fatalf("parser.Parse(%s) failed: %v", tc.in, iss.ToDisplayString()) } - out, err := Unparse(p.GetExpr(), p.GetSourceInfo()) + out, err := Unparse(p.GetExpr(), p.GetSourceInfo(), tc.formattingOptions...) + if err != nil { t.Fatalf("Unparse(%s) failed: %v", tc.in, err) } @@ -179,10 +367,18 @@ func TestUnparse(t *testing.T) { } func TestUnparseErrors(t *testing.T) { + validConstantExpression := &exprpb.Expr{ + ExprKind: &exprpb.Expr_ConstExpr{ + ConstExpr: &exprpb.Constant{ + ConstantKind: &exprpb.Constant_NullValue{}, + }, + }, + } tests := []struct { - name string - in *exprpb.Expr - err error + name string + in *exprpb.Expr + err error + formattingOptions []UnparserFormattingOption }{ {name: "empty_expr", in: &exprpb.Expr{}, err: errors.New("unsupported expression")}, { @@ -245,12 +441,36 @@ func TestUnparseErrors(t *testing.T) { }, err: errors.New("unsupported expression"), }, + { + name: "bad_formatting_option_wrap_column", + in: validConstantExpression, + err: errors.New("Invalid formatting option. Wrap column value must be greater than or equal to 1. Got 0 instead"), + formattingOptions: []UnparserFormattingOption{ + WrapColumn(0), + }, + }, + { + name: "bad_formatting_option_unsupported_operator", + in: validConstantExpression, + err: errors.New("Invalid formatting option. Unsupported operator: bogus"), + formattingOptions: []UnparserFormattingOption{ + WrapOnOperators("bogus"), + }, + }, + { + name: "bad_formatting_option_unary_operator", + in: validConstantExpression, + err: errors.New("Invalid formatting option. Unary operators are unsupported: " + operators.Negate), + formattingOptions: []UnparserFormattingOption{ + WrapOnOperators(operators.Negate), + }, + }, } for _, tst := range tests { tc := tst t.Run(tc.name, func(t *testing.T) { - out, err := Unparse(tc.in, &exprpb.SourceInfo{}) + out, err := Unparse(tc.in, &exprpb.SourceInfo{}, tc.formattingOptions...) if err == nil { t.Fatalf("Unparse(%v) got %v, wanted error %v", tc.in, out, tc.err) } From 7727e6cec8c5850ba2374b9a9575fd497fb548b9 Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Mon, 11 Jul 2022 17:29:24 +0000 Subject: [PATCH 2/7] Rename UnparserFormattingOption to UnparserOption, addressed other comments --- parser/unparser.go | 38 ++++++++------ parser/unparser_test.go | 112 +++++++++++++++++++++++----------------- 2 files changed, 86 insertions(+), 64 deletions(-) diff --git a/parser/unparser.go b/parser/unparser.go index ece704c8..3cdc658f 100644 --- a/parser/unparser.go +++ b/parser/unparser.go @@ -37,9 +37,10 @@ import ( // - Spacing around punctuation marks may be lost. // - Parentheses will only be applied when they affect operator precedence. // -// This function optionally takes in one or more UnparserFormattingOption to dictate the formatting of the output. -func Unparse(expr *exprpb.Expr, info *exprpb.SourceInfo, opts ...UnparserFormattingOption) (string, error) { - formattingOpts := &formattingOption{ +// This function optionally takes in one or more UnparserOption to alter the unparsing behavior, such as +// performing word wrapping on operators. +func Unparse(expr *exprpb.Expr, info *exprpb.SourceInfo, opts ...UnparserOption) (string, error) { + formattingOpts := &unparserOption{ wrapColumn: defaultWrapColumn, } @@ -66,7 +67,7 @@ func Unparse(expr *exprpb.Expr, info *exprpb.SourceInfo, opts ...UnparserFormatt type unparser struct { str strings.Builder info *exprpb.SourceInfo - options *formattingOption + options *unparserOption lastWrappedIndex int } @@ -472,7 +473,7 @@ func bytesToOctets(byteVal []byte) string { return b.String() } -// Takes in a function then inserts a newline based on the rules provided in formatting options +// wrapForOperator inserts a newline after operators configured in the formatting options. func (un *unparser) wrapForOperator(fun string) bool { _, wrapOperatorExists := un.options.operatorsToWrapOn[fun] lineLength := un.str.Len() - un.lastWrappedIndex @@ -489,18 +490,17 @@ var ( defaultWrapColumn = 80 ) -// UnparserFormattingOption is a funcitonal option for configuring the output formatting +// UnparserOption is a funcitonal option for configuring the output formatting // of the Unparse function. -type UnparserFormattingOption func(*formattingOption) (*formattingOption, error) +type UnparserOption func(*unparserOption) (*unparserOption, error) -// Internal representation of the UnparserFormattingOption type -type formattingOption struct { +// Internal representation of the UnparserOption type +type unparserOption struct { wrapColumn int // Defaults to 80 operatorsToWrapOn map[string]bool } -// WrapOnColumn returns an UnparserFormattingOption with the column limit set. -// Word wrapping will be performed when a line's length exceeds the limit +// WrapOnColumn wraps the output expression when its string length exceeds a specified limit // for operators set by WrapOnOperators function. // // Example usage: @@ -509,10 +509,14 @@ type formattingOption struct { // // This will insert a newline immediately after the logical AND operator for the below example input: // -// Input: a && b -// Output: a &&\nb -func WrapColumn(col int) UnparserFormattingOption { - return func(opt *formattingOption) (*formattingOption, error) { +// Input: +// 'my-principal-group' in request.auth.claims && request.auth.claims.iat > now - duration('5m') +// +// Output: +// 'my-principal-group' in request.auth.claims && +// request.auth.claims.iat > now - duration('5m') +func WrapOnColumn(col int) UnparserOption { + return func(opt *unparserOption) (*unparserOption, error) { if col < 1 { return nil, fmt.Errorf("Invalid formatting option. Wrap column value must be greater than or equal to 1. Got %v instead", col) } @@ -525,8 +529,8 @@ func WrapColumn(col int) UnparserFormattingOption { // Word wrapping is supported on non-unary symbolic operators. Refer to operators.go for the full list // // This will replace any previously supplied operators instead of merging them. -func WrapOnOperators(symbols ...string) UnparserFormattingOption { - return func(fmtOpt *formattingOption) (*formattingOption, error) { +func WrapOnOperators(symbols ...string) UnparserOption { + return func(fmtOpt *unparserOption) (*unparserOption, error) { fmtOpt.operatorsToWrapOn = make(map[string]bool) for _, symbol := range symbols { _, found := operators.FindReverse(symbol) diff --git a/parser/unparser_test.go b/parser/unparser_test.go index a20521cb..d68cc1ae 100644 --- a/parser/unparser_test.go +++ b/parser/unparser_test.go @@ -21,6 +21,7 @@ import ( "github.com/google/cel-go/common" "github.com/google/cel-go/common/operators" + "google.golang.org/protobuf/proto" exprpb "google.golang.org/genproto/googleapis/api/expr/v1alpha1" @@ -32,7 +33,7 @@ func TestUnparse(t *testing.T) { in string out interface{} requiresMacroCalls bool - formattingOptions []UnparserFormattingOption + formattingOptions []UnparserOption }{ {name: "call_add", in: `a + b - c`}, {name: "call_and", in: `a && b && c && d && e`}, @@ -147,16 +148,16 @@ func TestUnparse(t *testing.T) { name: "call_no_wrap_no_operators", in: "a + b + c + d", out: "a + b + c + d", - formattingOptions: []UnparserFormattingOption{ - WrapColumn(3), + formattingOptions: []UnparserOption{ + WrapOnColumn(3), }, }, { name: "call_no_wrap_short_line", in: "a + b + c + d", out: "a + b + c + d", - formattingOptions: []UnparserFormattingOption{ - WrapColumn(12), + formattingOptions: []UnparserOption{ + WrapOnColumn(12), WrapOnOperators(operators.Add), }, }, @@ -166,8 +167,8 @@ func TestUnparse(t *testing.T) { name: "call_wrap_add", in: "a + b - d * e", out: "a +\nb - d * e", - formattingOptions: []UnparserFormattingOption{ - WrapColumn(3), + formattingOptions: []UnparserOption{ + WrapOnColumn(3), WrapOnOperators(operators.Add), }, }, @@ -175,8 +176,8 @@ func TestUnparse(t *testing.T) { name: "call_wrap_add_and_subtract", in: "a * b + c - d * e", out: "a * b +\nc -\nd * e", - formattingOptions: []UnparserFormattingOption{ - WrapColumn(3), + formattingOptions: []UnparserOption{ + WrapOnColumn(3), WrapOnOperators(operators.Add, operators.Subtract), }, }, @@ -184,8 +185,8 @@ func TestUnparse(t *testing.T) { name: "call_wrap_and", in: "a && b && c && d && e", out: "a &&\nb &&\nc &&\nd &&\ne", - formattingOptions: []UnparserFormattingOption{ - WrapColumn(3), + formattingOptions: []UnparserOption{ + WrapOnColumn(3), WrapOnOperators(operators.LogicalAnd), }, }, @@ -193,8 +194,8 @@ func TestUnparse(t *testing.T) { name: "call_wrap_and_2", in: "a && b", out: "a &&\nb", - formattingOptions: []UnparserFormattingOption{ - WrapColumn(3), + formattingOptions: []UnparserOption{ + WrapOnColumn(3), WrapOnOperators(operators.LogicalAnd), }, }, @@ -202,8 +203,8 @@ func TestUnparse(t *testing.T) { name: "call_wrap_conditional", in: "a ? b : c ? d : e", out: "a ?\nb : (c ?\nd : e)", - formattingOptions: []UnparserFormattingOption{ - WrapColumn(3), + formattingOptions: []UnparserOption{ + WrapOnColumn(3), WrapOnOperators(operators.Conditional), }, }, @@ -211,8 +212,8 @@ func TestUnparse(t *testing.T) { name: "call_wrap_or", in: "a || b || c || d || e", out: "a ||\nb ||\nc ||\nd ||\ne", - formattingOptions: []UnparserFormattingOption{ - WrapColumn(3), + formattingOptions: []UnparserOption{ + WrapOnColumn(3), WrapOnOperators(operators.LogicalOr), }, }, @@ -220,8 +221,8 @@ func TestUnparse(t *testing.T) { name: "call_wrap_equals", in: "a == b == c == d == e", out: "a ==\nb ==\nc ==\nd ==\ne", - formattingOptions: []UnparserFormattingOption{ - WrapColumn(3), + formattingOptions: []UnparserOption{ + WrapOnColumn(3), WrapOnOperators(operators.Equals), }, }, @@ -229,8 +230,8 @@ func TestUnparse(t *testing.T) { name: "call_wrap_greater", in: "a > b > c > d > e", out: "a >\nb >\nc >\nd >\ne", - formattingOptions: []UnparserFormattingOption{ - WrapColumn(3), + formattingOptions: []UnparserOption{ + WrapOnColumn(3), WrapOnOperators(operators.Greater), }, }, @@ -238,8 +239,8 @@ func TestUnparse(t *testing.T) { name: "call_wrap_greater_equals", in: "a >= b >= c >= d >= e", out: "a >=\nb >=\nc >=\nd >=\ne", - formattingOptions: []UnparserFormattingOption{ - WrapColumn(3), + formattingOptions: []UnparserOption{ + WrapOnColumn(3), WrapOnOperators(operators.GreaterEquals), }, }, @@ -247,8 +248,8 @@ func TestUnparse(t *testing.T) { name: "call_wrap_in", in: "a in b in c in d in e", out: "a in\nb in\nc in\nd in\ne", - formattingOptions: []UnparserFormattingOption{ - WrapColumn(3), + formattingOptions: []UnparserOption{ + WrapOnColumn(3), WrapOnOperators(operators.In), }, }, @@ -256,8 +257,8 @@ func TestUnparse(t *testing.T) { name: "call_wrap_less", in: "a < b < c < d < e", out: "a <\nb <\nc <\nd <\ne", - formattingOptions: []UnparserFormattingOption{ - WrapColumn(3), + formattingOptions: []UnparserOption{ + WrapOnColumn(3), WrapOnOperators(operators.Less), }, }, @@ -265,8 +266,8 @@ func TestUnparse(t *testing.T) { name: "call_wrap_less_equals", in: "a <= b <= c <= d <= e", out: "a <=\nb <=\nc <=\nd <=\ne", - formattingOptions: []UnparserFormattingOption{ - WrapColumn(3), + formattingOptions: []UnparserOption{ + WrapOnColumn(3), WrapOnOperators(operators.LessEquals), }, }, @@ -274,8 +275,8 @@ func TestUnparse(t *testing.T) { name: "call_wrap_not_equals", in: "a != b != c != d != e", out: "a !=\nb !=\nc !=\nd !=\ne", - formattingOptions: []UnparserFormattingOption{ - WrapColumn(3), + formattingOptions: []UnparserOption{ + WrapOnColumn(3), WrapOnOperators(operators.NotEquals), }, }, @@ -283,8 +284,8 @@ func TestUnparse(t *testing.T) { name: "call_wrap_divide", in: "a / b / c / d / e", out: "a /\nb /\nc /\nd /\ne", - formattingOptions: []UnparserFormattingOption{ - WrapColumn(3), + formattingOptions: []UnparserOption{ + WrapOnColumn(3), WrapOnOperators(operators.Divide), }, }, @@ -292,8 +293,8 @@ func TestUnparse(t *testing.T) { name: "call_wrap_modulo", in: "a % b % c % d % e", out: "a %\nb %\nc %\nd %\ne", - formattingOptions: []UnparserFormattingOption{ - WrapColumn(3), + formattingOptions: []UnparserOption{ + WrapOnColumn(3), WrapOnOperators(operators.Modulo), }, }, @@ -301,8 +302,8 @@ func TestUnparse(t *testing.T) { name: "call_wrap_multiply", in: "a * b * c * d * e", out: "a *\nb *\nc *\nd *\ne", - formattingOptions: []UnparserFormattingOption{ - WrapColumn(3), + formattingOptions: []UnparserOption{ + WrapOnColumn(3), WrapOnOperators(operators.Multiply), }, }, @@ -310,8 +311,17 @@ func TestUnparse(t *testing.T) { name: "call_wrap_and_long_variables", in: "longVariableA && longVariableB && longVariableC", out: "longVariableA &&\nlongVariableB &&\nlongVariableC", - formattingOptions: []UnparserFormattingOption{ - WrapColumn(3), + formattingOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.LogicalAnd), + }, + }, + { + name: "call_wrap_and_long_input", + in: `"my-principal-group" in request.auth.claims && request.auth.claims.iat > now - duration("5m")`, + out: `"my-principal-group" in request.auth.claims &&` + "\n" + `request.auth.claims.iat > now - duration("5m")`, + formattingOptions: []UnparserOption{ + WrapOnColumn(40), WrapOnOperators(operators.LogicalAnd), }, }, @@ -320,8 +330,8 @@ func TestUnparse(t *testing.T) { in: "[1, 2, 3].map(x, x >= 2, x * 4).filter(x, x <= 10)", out: "[1, 2, 3].map(x, x >=\n2, x * 4).filter(x, x <=\n10)", requiresMacroCalls: true, - formattingOptions: []UnparserFormattingOption{ - WrapColumn(3), + formattingOptions: []UnparserOption{ + WrapOnColumn(3), WrapOnOperators(operators.GreaterEquals, operators.LessEquals), }, }, @@ -378,7 +388,7 @@ func TestUnparseErrors(t *testing.T) { name string in *exprpb.Expr err error - formattingOptions []UnparserFormattingOption + formattingOptions []UnparserOption }{ {name: "empty_expr", in: &exprpb.Expr{}, err: errors.New("unsupported expression")}, { @@ -442,18 +452,26 @@ func TestUnparseErrors(t *testing.T) { err: errors.New("unsupported expression"), }, { - name: "bad_formatting_option_wrap_column", + name: "bad_formatting_option_wrap_column_zero", in: validConstantExpression, err: errors.New("Invalid formatting option. Wrap column value must be greater than or equal to 1. Got 0 instead"), - formattingOptions: []UnparserFormattingOption{ - WrapColumn(0), + formattingOptions: []UnparserOption{ + WrapOnColumn(0), + }, + }, + { + name: "bad_formatting_option_wrap_column_negative", + in: validConstantExpression, + err: errors.New("Invalid formatting option. Wrap column value must be greater than or equal to 1. Got -1 instead"), + formattingOptions: []UnparserOption{ + WrapOnColumn(-1), }, }, { name: "bad_formatting_option_unsupported_operator", in: validConstantExpression, err: errors.New("Invalid formatting option. Unsupported operator: bogus"), - formattingOptions: []UnparserFormattingOption{ + formattingOptions: []UnparserOption{ WrapOnOperators("bogus"), }, }, @@ -461,7 +479,7 @@ func TestUnparseErrors(t *testing.T) { name: "bad_formatting_option_unary_operator", in: validConstantExpression, err: errors.New("Invalid formatting option. Unary operators are unsupported: " + operators.Negate), - formattingOptions: []UnparserFormattingOption{ + formattingOptions: []UnparserOption{ WrapOnOperators(operators.Negate), }, }, From 5fa13d0b1be7ea0155d46a2788f3017f9cbd8835 Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Mon, 11 Jul 2022 18:13:58 +0000 Subject: [PATCH 3/7] Add the capability to wrap before or after the specified operators --- parser/unparser.go | 52 +++++++++++++++++++++++++++-------------- parser/unparser_test.go | 13 +++++++++-- 2 files changed, 45 insertions(+), 20 deletions(-) diff --git a/parser/unparser.go b/parser/unparser.go index 3cdc658f..eb2304e1 100644 --- a/parser/unparser.go +++ b/parser/unparser.go @@ -41,7 +41,8 @@ import ( // performing word wrapping on operators. func Unparse(expr *exprpb.Expr, info *exprpb.SourceInfo, opts ...UnparserOption) (string, error) { formattingOpts := &unparserOption{ - wrapColumn: defaultWrapColumn, + wrapColumn: defaultWrapColumn, + wrapAfterColumnLimit: defaultWrapAfterColumnLimit, } var err error @@ -156,11 +157,7 @@ func (un *unparser) visitCallBinary(expr *exprpb.Expr) error { return fmt.Errorf("cannot unmangle operator: %s", fun) } - un.str.WriteString(" ") - un.str.WriteString(unmangled) - if !un.wrapForOperator(fun) { - un.str.WriteString(" ") - } + un.writeOperatorWithWrapping(fun, unmangled) return un.visitMaybeNested(rhs, rhsParen) } @@ -174,10 +171,7 @@ func (un *unparser) visitCallConditional(expr *exprpb.Expr) error { if err != nil { return err } - un.str.WriteString(" ?") - if !un.wrapForOperator(operators.Conditional) { - un.str.WriteString(" ") - } + un.writeOperatorWithWrapping(operators.Conditional, "?") // add parens if operand is a conditional itself. nested = isSamePrecedence(operators.Conditional, args[1]) || @@ -473,21 +467,42 @@ func bytesToOctets(byteVal []byte) string { return b.String() } -// wrapForOperator inserts a newline after operators configured in the formatting options. -func (un *unparser) wrapForOperator(fun string) bool { +// writeOperatorWithWrapping outputs the operator and inserts a newline for operators configured +// in the formatting options. +func (un *unparser) writeOperatorWithWrapping(fun string, unmangled string) bool { _, wrapOperatorExists := un.options.operatorsToWrapOn[fun] - lineLength := un.str.Len() - un.lastWrappedIndex + lineLength := un.str.Len() - un.lastWrappedIndex + len(fun) + if wrapOperatorExists && lineLength >= un.options.wrapColumn { un.lastWrappedIndex = un.str.Len() - un.str.WriteString("\n") + // wrapAfterColumnLimit flag dictates whether the newline is placed + // before or after the operator + if un.options.wrapAfterColumnLimit { + // Input: a && b + // Output: a &&\nb + un.str.WriteString(" ") + un.str.WriteString(unmangled) + un.str.WriteString("\n") + } else { + // Input: a && b + // Output: a\n&& b + un.str.WriteString("\n") + un.str.WriteString(unmangled) + un.str.WriteString(" ") + } return true + } else { + un.str.WriteString(" ") + un.str.WriteString(unmangled) + un.str.WriteString(" ") } return false } // Defined defaults for the formatting options -var ( - defaultWrapColumn = 80 +const ( + defaultWrapColumn = 80 + defaultWrapAfterColumnLimit = true ) // UnparserOption is a funcitonal option for configuring the output formatting @@ -496,8 +511,9 @@ type UnparserOption func(*unparserOption) (*unparserOption, error) // Internal representation of the UnparserOption type type unparserOption struct { - wrapColumn int // Defaults to 80 - operatorsToWrapOn map[string]bool + wrapColumn int + operatorsToWrapOn map[string]bool + wrapAfterColumnLimit bool } // WrapOnColumn wraps the output expression when its string length exceeds a specified limit diff --git a/parser/unparser_test.go b/parser/unparser_test.go index d68cc1ae..53490d1a 100644 --- a/parser/unparser_test.go +++ b/parser/unparser_test.go @@ -153,11 +153,20 @@ func TestUnparse(t *testing.T) { }, }, { - name: "call_no_wrap_short_line", + name: "call_no_wrap_column_limit_large_val", in: "a + b + c + d", out: "a + b + c + d", formattingOptions: []UnparserOption{ - WrapOnColumn(12), + WrapOnColumn(1000), + WrapOnOperators(operators.Add), + }, + }, + { + name: "call_no_wrap_column_limit_equal_length_to_input", + in: "a + b + c + d", + out: "a + b + c + d", + formattingOptions: []UnparserOption{ + WrapOnColumn(13), WrapOnOperators(operators.Add), }, }, From 31abeaccea39c8a94926e2075affa4631bd40f12 Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Mon, 11 Jul 2022 19:21:40 +0000 Subject: [PATCH 4/7] Expose WrapAfterColumnLimit option --- parser/unparser.go | 40 ++++++++++++++++++----- parser/unparser_test.go | 71 ++++++++++++++++++++++++++++++++++------- 2 files changed, 92 insertions(+), 19 deletions(-) diff --git a/parser/unparser.go b/parser/unparser.go index eb2304e1..f68bab9c 100644 --- a/parser/unparser.go +++ b/parser/unparser.go @@ -38,7 +38,7 @@ import ( // - Parentheses will only be applied when they affect operator precedence. // // This function optionally takes in one or more UnparserOption to alter the unparsing behavior, such as -// performing word wrapping on operators. +// performing word wrapping on expressions. func Unparse(expr *exprpb.Expr, info *exprpb.SourceInfo, opts ...UnparserOption) (string, error) { formattingOpts := &unparserOption{ wrapColumn: defaultWrapColumn, @@ -517,11 +517,11 @@ type unparserOption struct { } // WrapOnColumn wraps the output expression when its string length exceeds a specified limit -// for operators set by WrapOnOperators function. +// for operators set by WrapOnOperators function or by default, "&&" and "||" will be wrapped. // // Example usage: // -// Unparse(expr, sourceInfo, WrapColumn(3), WrapOnOperators(Operators.LogicalAnd)) +// Unparse(expr, sourceInfo, WrapColumn(40), WrapOnOperators(Operators.LogicalAnd)) // // This will insert a newline immediately after the logical AND operator for the below example input: // @@ -541,13 +541,15 @@ func WrapOnColumn(col int) UnparserOption { } } -// WrapOnOperators returns an UnparserFormattedOption with operators to perform word wrapping on. +// WrapOnOperators specifies which operators to perform word wrapping on an output expression when its string length +// exceeds the column limit set by WrapOnColumn function. +// // Word wrapping is supported on non-unary symbolic operators. Refer to operators.go for the full list // // This will replace any previously supplied operators instead of merging them. func WrapOnOperators(symbols ...string) UnparserOption { - return func(fmtOpt *unparserOption) (*unparserOption, error) { - fmtOpt.operatorsToWrapOn = make(map[string]bool) + return func(opt *unparserOption) (*unparserOption, error) { + opt.operatorsToWrapOn = make(map[string]bool) for _, symbol := range symbols { _, found := operators.FindReverse(symbol) if !found { @@ -558,9 +560,31 @@ func WrapOnOperators(symbols ...string) UnparserOption { return nil, fmt.Errorf("Invalid formatting option. Unary operators are unsupported: %s", symbol) } - fmtOpt.operatorsToWrapOn[symbol] = true + opt.operatorsToWrapOn[symbol] = true } - return fmtOpt, nil + return opt, nil + } +} + +// WrapAfterColumnLimit dictates whether to insert a newline before or after the specified operator +// when word wrapping is performed. +// +// Example usage: +// +// Unparse(expr, sourceInfo, WrapColumn(40), WrapOnOperators(Operators.LogicalAnd), WrapAfterColumnLimit(false)) +// +// This will insert a newline immediately before the logical AND operator for the below example input: +// +// Input: +// 'my-principal-group' in request.auth.claims && request.auth.claims.iat > now - duration('5m') +// +// Output: +// 'my-principal-group' in request.auth.claims && +// request.auth.claims.iat > now - duration('5m') +func WrapAfterColumnLimit(wrapAfter bool) UnparserOption { + return func(opt *unparserOption) (*unparserOption, error) { + opt.wrapAfterColumnLimit = wrapAfter + return opt, nil } } diff --git a/parser/unparser_test.go b/parser/unparser_test.go index 53490d1a..495c0692 100644 --- a/parser/unparser_test.go +++ b/parser/unparser_test.go @@ -182,7 +182,7 @@ func TestUnparse(t *testing.T) { }, }, { - name: "call_wrap_add_and_subtract", + name: "call_wrap_add_subtract", in: "a * b + c - d * e", out: "a * b +\nc -\nd * e", formattingOptions: []UnparserOption{ @@ -191,7 +191,16 @@ func TestUnparse(t *testing.T) { }, }, { - name: "call_wrap_and", + name: "call_wrap_add_subtract", + in: "a * b + c - d * e", + out: "a * b +\nc -\nd * e", + formattingOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.Add, operators.Subtract), + }, + }, + { + name: "call_wrap_logical_and", in: "a && b && c && d && e", out: "a &&\nb &&\nc &&\nd &&\ne", formattingOptions: []UnparserOption{ @@ -200,7 +209,7 @@ func TestUnparse(t *testing.T) { }, }, { - name: "call_wrap_and_2", + name: "call_wrap_logical_and_2", in: "a && b", out: "a &&\nb", formattingOptions: []UnparserOption{ @@ -317,7 +326,7 @@ func TestUnparse(t *testing.T) { }, }, { - name: "call_wrap_and_long_variables", + name: "call_wrap_logical_and_long_variables", in: "longVariableA && longVariableB && longVariableC", out: "longVariableA &&\nlongVariableB &&\nlongVariableC", formattingOptions: []UnparserOption{ @@ -326,7 +335,47 @@ func TestUnparse(t *testing.T) { }, }, { - name: "call_wrap_and_long_input", + name: "comp_chained_wrap_comparisons", + in: "[1, 2, 3].map(x, x >= 2, x * 4).filter(x, x <= 10)", + out: "[1, 2, 3].map(x, x >=\n2, x * 4).filter(x, x <=\n10)", + requiresMacroCalls: true, + formattingOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.GreaterEquals, operators.LessEquals), + }, + }, + { + name: "call_wrap_before_add", + in: "a + b - d * e", + out: "a\n+ b - d * e", + formattingOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.Add), + WrapAfterColumnLimit(false), + }, + }, + { + name: "call_wrap_before_add_subtract", + in: "a * b + c - d * e", + out: "a * b\n+ c\n- d * e", + formattingOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.Add, operators.Subtract), + WrapAfterColumnLimit(false), + }, + }, + { + name: "call_wrap_logical_and_long_variables", + in: "longVariableA && longVariableB && longVariableC", + out: "longVariableA\n&& longVariableB\n&& longVariableC", + formattingOptions: []UnparserOption{ + WrapOnColumn(3), + WrapOnOperators(operators.LogicalAnd), + WrapAfterColumnLimit(false), + }, + }, + { + name: "call_wrap_logical_and_long_input", in: `"my-principal-group" in request.auth.claims && request.auth.claims.iat > now - duration("5m")`, out: `"my-principal-group" in request.auth.claims &&` + "\n" + `request.auth.claims.iat > now - duration("5m")`, formattingOptions: []UnparserOption{ @@ -335,13 +384,13 @@ func TestUnparse(t *testing.T) { }, }, { - name: "comp_chained_wrap_comparisons", - in: "[1, 2, 3].map(x, x >= 2, x * 4).filter(x, x <= 10)", - out: "[1, 2, 3].map(x, x >=\n2, x * 4).filter(x, x <=\n10)", - requiresMacroCalls: true, + name: "call_wrap_before_logical_and_long_input", + in: `"my-principal-group" in request.auth.claims && request.auth.claims.iat > now - duration("5m")`, + out: `"my-principal-group" in request.auth.claims` + "\n" + `&& request.auth.claims.iat > now - duration("5m")`, formattingOptions: []UnparserOption{ - WrapOnColumn(3), - WrapOnOperators(operators.GreaterEquals, operators.LessEquals), + WrapOnColumn(40), + WrapOnOperators(operators.LogicalAnd), + WrapAfterColumnLimit(false), }, }, } From 58de03afc0e179179bcbbdc43b4db529297fb294 Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Mon, 11 Jul 2022 20:32:21 +0000 Subject: [PATCH 5/7] Set LogicalAnd and LogicalOr as default operators for wrapping --- parser/unparser.go | 14 ++++++++++---- parser/unparser_test.go | 23 +++++++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/parser/unparser.go b/parser/unparser.go index f68bab9c..83b956f1 100644 --- a/parser/unparser.go +++ b/parser/unparser.go @@ -43,6 +43,7 @@ func Unparse(expr *exprpb.Expr, info *exprpb.SourceInfo, opts ...UnparserOption) formattingOpts := &unparserOption{ wrapColumn: defaultWrapColumn, wrapAfterColumnLimit: defaultWrapAfterColumnLimit, + operatorsToWrapOn: defaultOperatorsToWrapOn, } var err error @@ -500,9 +501,13 @@ func (un *unparser) writeOperatorWithWrapping(fun string, unmangled string) bool } // Defined defaults for the formatting options -const ( +var ( defaultWrapColumn = 80 defaultWrapAfterColumnLimit = true + defaultOperatorsToWrapOn = map[string]bool{ + operators.LogicalAnd: true, + operators.LogicalOr: true, + } ) // UnparserOption is a funcitonal option for configuring the output formatting @@ -574,14 +579,15 @@ func WrapOnOperators(symbols ...string) UnparserOption { // // Unparse(expr, sourceInfo, WrapColumn(40), WrapOnOperators(Operators.LogicalAnd), WrapAfterColumnLimit(false)) // -// This will insert a newline immediately before the logical AND operator for the below example input: +// This will insert a newline immediately before the logical AND operator for the below example input, ensuring +// that the length of a line never exceeds the specified column limit: // // Input: // 'my-principal-group' in request.auth.claims && request.auth.claims.iat > now - duration('5m') // // Output: -// 'my-principal-group' in request.auth.claims && -// request.auth.claims.iat > now - duration('5m') +// 'my-principal-group' in request.auth.claims +// && request.auth.claims.iat > now - duration('5m') func WrapAfterColumnLimit(wrapAfter bool) UnparserOption { return func(opt *unparserOption) (*unparserOption, error) { opt.wrapAfterColumnLimit = wrapAfter diff --git a/parser/unparser_test.go b/parser/unparser_test.go index 495c0692..c039dd8c 100644 --- a/parser/unparser_test.go +++ b/parser/unparser_test.go @@ -393,6 +393,29 @@ func TestUnparse(t *testing.T) { WrapAfterColumnLimit(false), }, }, + { + // By default: + // - Column limit is at 80 + // - && and || are wrapped + // - Wrapping occurs after the symbol + name: "call_wrap_default", + in: `jwt.extra_claims.filter(c, c.startsWith("group")).all(c, jwt.extra_claims[c].all(g, g.endsWith("@acme.co"))) && jwt.extra_claims.exists(c, c.startsWith("group")) || request.auth.claims.group == "admin" || request.auth.principal == "user:me@acme.co"`, + out: `jwt.extra_claims.filter(c, c.startsWith("group")).all(c, jwt.extra_claims[c].all(g, g.endsWith("@acme.co"))) &&` + + "\n" + + `jwt.extra_claims.exists(c, c.startsWith("group")) || request.auth.claims.group == "admin" ||` + + "\n" + + `request.auth.principal == "user:me@acme.co"`, + requiresMacroCalls: true, + }, + { + // && and || are wrapped by default if only the column limit is specified + name: "call_wrap_default_operators", + in: "longVariableA && longVariableB || longVariableC + longVariableD - longVariableE", + out: "longVariableA &&\nlongVariableB ||\nlongVariableC + longVariableD - longVariableE", + formattingOptions: []UnparserOption{ + WrapOnColumn(3), + }, + }, } for _, tst := range tests { From a6985bda9339c0dbb3745e8874dfcaf1727e1752 Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Mon, 11 Jul 2022 20:59:01 +0000 Subject: [PATCH 6/7] Use consistent terminology formatting option -> unparsing option --- parser/unparser.go | 18 ++++---- parser/unparser_test.go | 98 ++++++++++++++++++++--------------------- 2 files changed, 58 insertions(+), 58 deletions(-) diff --git a/parser/unparser.go b/parser/unparser.go index 83b956f1..48aa3122 100644 --- a/parser/unparser.go +++ b/parser/unparser.go @@ -40,7 +40,7 @@ import ( // This function optionally takes in one or more UnparserOption to alter the unparsing behavior, such as // performing word wrapping on expressions. func Unparse(expr *exprpb.Expr, info *exprpb.SourceInfo, opts ...UnparserOption) (string, error) { - formattingOpts := &unparserOption{ + unparserOpts := &unparserOption{ wrapColumn: defaultWrapColumn, wrapAfterColumnLimit: defaultWrapAfterColumnLimit, operatorsToWrapOn: defaultOperatorsToWrapOn, @@ -48,7 +48,7 @@ func Unparse(expr *exprpb.Expr, info *exprpb.SourceInfo, opts ...UnparserOption) var err error for _, opt := range opts { - formattingOpts, err = opt(formattingOpts) + unparserOpts, err = opt(unparserOpts) if err != nil { return "", err } @@ -56,7 +56,7 @@ func Unparse(expr *exprpb.Expr, info *exprpb.SourceInfo, opts ...UnparserOption) un := &unparser{ info: info, - options: formattingOpts, + options: unparserOpts, } err = un.visit(expr) if err != nil { @@ -469,7 +469,7 @@ func bytesToOctets(byteVal []byte) string { } // writeOperatorWithWrapping outputs the operator and inserts a newline for operators configured -// in the formatting options. +// in the unparser options. func (un *unparser) writeOperatorWithWrapping(fun string, unmangled string) bool { _, wrapOperatorExists := un.options.operatorsToWrapOn[fun] lineLength := un.str.Len() - un.lastWrappedIndex + len(fun) @@ -500,7 +500,7 @@ func (un *unparser) writeOperatorWithWrapping(fun string, unmangled string) bool return false } -// Defined defaults for the formatting options +// Defined defaults for the unparser options var ( defaultWrapColumn = 80 defaultWrapAfterColumnLimit = true @@ -510,7 +510,7 @@ var ( } ) -// UnparserOption is a funcitonal option for configuring the output formatting +// UnparserOption is a functional option for configuring the output formatting // of the Unparse function. type UnparserOption func(*unparserOption) (*unparserOption, error) @@ -539,7 +539,7 @@ type unparserOption struct { func WrapOnColumn(col int) UnparserOption { return func(opt *unparserOption) (*unparserOption, error) { if col < 1 { - return nil, fmt.Errorf("Invalid formatting option. Wrap column value must be greater than or equal to 1. Got %v instead", col) + return nil, fmt.Errorf("Invalid unparser option. Wrap column value must be greater than or equal to 1. Got %v instead", col) } opt.wrapColumn = col return opt, nil @@ -558,11 +558,11 @@ func WrapOnOperators(symbols ...string) UnparserOption { for _, symbol := range symbols { _, found := operators.FindReverse(symbol) if !found { - return nil, fmt.Errorf("Invalid formatting option. Unsupported operator: %s", symbol) + return nil, fmt.Errorf("Invalid unparser option. Unsupported operator: %s", symbol) } arity := operators.Arity(symbol) if arity < 2 { - return nil, fmt.Errorf("Invalid formatting option. Unary operators are unsupported: %s", symbol) + return nil, fmt.Errorf("Invalid unparser option. Unary operators are unsupported: %s", symbol) } opt.operatorsToWrapOn[symbol] = true diff --git a/parser/unparser_test.go b/parser/unparser_test.go index c039dd8c..fad688e9 100644 --- a/parser/unparser_test.go +++ b/parser/unparser_test.go @@ -33,7 +33,7 @@ func TestUnparse(t *testing.T) { in string out interface{} requiresMacroCalls bool - formattingOptions []UnparserOption + unparserOptions []UnparserOption }{ {name: "call_add", in: `a + b - c`}, {name: "call_and", in: `a && b && c && d && e`}, @@ -143,12 +143,12 @@ func TestUnparse(t *testing.T) { }, // These expressions will not be wrapped because they haven't met the - // conditions required by the provided formatting options + // conditions required by the provided unparser options { name: "call_no_wrap_no_operators", in: "a + b + c + d", out: "a + b + c + d", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(3), }, }, @@ -156,7 +156,7 @@ func TestUnparse(t *testing.T) { name: "call_no_wrap_column_limit_large_val", in: "a + b + c + d", out: "a + b + c + d", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(1000), WrapOnOperators(operators.Add), }, @@ -165,18 +165,18 @@ func TestUnparse(t *testing.T) { name: "call_no_wrap_column_limit_equal_length_to_input", in: "a + b + c + d", out: "a + b + c + d", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(13), WrapOnOperators(operators.Add), }, }, - // These expressions will be formatted based on the formatting options provided + // These expressions will be formatted based on the unparser options provided { name: "call_wrap_add", in: "a + b - d * e", out: "a +\nb - d * e", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(3), WrapOnOperators(operators.Add), }, @@ -185,7 +185,7 @@ func TestUnparse(t *testing.T) { name: "call_wrap_add_subtract", in: "a * b + c - d * e", out: "a * b +\nc -\nd * e", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(3), WrapOnOperators(operators.Add, operators.Subtract), }, @@ -194,7 +194,7 @@ func TestUnparse(t *testing.T) { name: "call_wrap_add_subtract", in: "a * b + c - d * e", out: "a * b +\nc -\nd * e", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(3), WrapOnOperators(operators.Add, operators.Subtract), }, @@ -203,7 +203,7 @@ func TestUnparse(t *testing.T) { name: "call_wrap_logical_and", in: "a && b && c && d && e", out: "a &&\nb &&\nc &&\nd &&\ne", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(3), WrapOnOperators(operators.LogicalAnd), }, @@ -212,7 +212,7 @@ func TestUnparse(t *testing.T) { name: "call_wrap_logical_and_2", in: "a && b", out: "a &&\nb", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(3), WrapOnOperators(operators.LogicalAnd), }, @@ -221,7 +221,7 @@ func TestUnparse(t *testing.T) { name: "call_wrap_conditional", in: "a ? b : c ? d : e", out: "a ?\nb : (c ?\nd : e)", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(3), WrapOnOperators(operators.Conditional), }, @@ -230,7 +230,7 @@ func TestUnparse(t *testing.T) { name: "call_wrap_or", in: "a || b || c || d || e", out: "a ||\nb ||\nc ||\nd ||\ne", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(3), WrapOnOperators(operators.LogicalOr), }, @@ -239,7 +239,7 @@ func TestUnparse(t *testing.T) { name: "call_wrap_equals", in: "a == b == c == d == e", out: "a ==\nb ==\nc ==\nd ==\ne", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(3), WrapOnOperators(operators.Equals), }, @@ -248,7 +248,7 @@ func TestUnparse(t *testing.T) { name: "call_wrap_greater", in: "a > b > c > d > e", out: "a >\nb >\nc >\nd >\ne", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(3), WrapOnOperators(operators.Greater), }, @@ -257,7 +257,7 @@ func TestUnparse(t *testing.T) { name: "call_wrap_greater_equals", in: "a >= b >= c >= d >= e", out: "a >=\nb >=\nc >=\nd >=\ne", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(3), WrapOnOperators(operators.GreaterEquals), }, @@ -266,7 +266,7 @@ func TestUnparse(t *testing.T) { name: "call_wrap_in", in: "a in b in c in d in e", out: "a in\nb in\nc in\nd in\ne", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(3), WrapOnOperators(operators.In), }, @@ -275,7 +275,7 @@ func TestUnparse(t *testing.T) { name: "call_wrap_less", in: "a < b < c < d < e", out: "a <\nb <\nc <\nd <\ne", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(3), WrapOnOperators(operators.Less), }, @@ -284,7 +284,7 @@ func TestUnparse(t *testing.T) { name: "call_wrap_less_equals", in: "a <= b <= c <= d <= e", out: "a <=\nb <=\nc <=\nd <=\ne", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(3), WrapOnOperators(operators.LessEquals), }, @@ -293,7 +293,7 @@ func TestUnparse(t *testing.T) { name: "call_wrap_not_equals", in: "a != b != c != d != e", out: "a !=\nb !=\nc !=\nd !=\ne", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(3), WrapOnOperators(operators.NotEquals), }, @@ -302,7 +302,7 @@ func TestUnparse(t *testing.T) { name: "call_wrap_divide", in: "a / b / c / d / e", out: "a /\nb /\nc /\nd /\ne", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(3), WrapOnOperators(operators.Divide), }, @@ -311,7 +311,7 @@ func TestUnparse(t *testing.T) { name: "call_wrap_modulo", in: "a % b % c % d % e", out: "a %\nb %\nc %\nd %\ne", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(3), WrapOnOperators(operators.Modulo), }, @@ -320,7 +320,7 @@ func TestUnparse(t *testing.T) { name: "call_wrap_multiply", in: "a * b * c * d * e", out: "a *\nb *\nc *\nd *\ne", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(3), WrapOnOperators(operators.Multiply), }, @@ -329,7 +329,7 @@ func TestUnparse(t *testing.T) { name: "call_wrap_logical_and_long_variables", in: "longVariableA && longVariableB && longVariableC", out: "longVariableA &&\nlongVariableB &&\nlongVariableC", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(3), WrapOnOperators(operators.LogicalAnd), }, @@ -339,7 +339,7 @@ func TestUnparse(t *testing.T) { in: "[1, 2, 3].map(x, x >= 2, x * 4).filter(x, x <= 10)", out: "[1, 2, 3].map(x, x >=\n2, x * 4).filter(x, x <=\n10)", requiresMacroCalls: true, - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(3), WrapOnOperators(operators.GreaterEquals, operators.LessEquals), }, @@ -348,7 +348,7 @@ func TestUnparse(t *testing.T) { name: "call_wrap_before_add", in: "a + b - d * e", out: "a\n+ b - d * e", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(3), WrapOnOperators(operators.Add), WrapAfterColumnLimit(false), @@ -358,7 +358,7 @@ func TestUnparse(t *testing.T) { name: "call_wrap_before_add_subtract", in: "a * b + c - d * e", out: "a * b\n+ c\n- d * e", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(3), WrapOnOperators(operators.Add, operators.Subtract), WrapAfterColumnLimit(false), @@ -368,7 +368,7 @@ func TestUnparse(t *testing.T) { name: "call_wrap_logical_and_long_variables", in: "longVariableA && longVariableB && longVariableC", out: "longVariableA\n&& longVariableB\n&& longVariableC", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(3), WrapOnOperators(operators.LogicalAnd), WrapAfterColumnLimit(false), @@ -378,7 +378,7 @@ func TestUnparse(t *testing.T) { name: "call_wrap_logical_and_long_input", in: `"my-principal-group" in request.auth.claims && request.auth.claims.iat > now - duration("5m")`, out: `"my-principal-group" in request.auth.claims &&` + "\n" + `request.auth.claims.iat > now - duration("5m")`, - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(40), WrapOnOperators(operators.LogicalAnd), }, @@ -387,7 +387,7 @@ func TestUnparse(t *testing.T) { name: "call_wrap_before_logical_and_long_input", in: `"my-principal-group" in request.auth.claims && request.auth.claims.iat > now - duration("5m")`, out: `"my-principal-group" in request.auth.claims` + "\n" + `&& request.auth.claims.iat > now - duration("5m")`, - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(40), WrapOnOperators(operators.LogicalAnd), WrapAfterColumnLimit(false), @@ -412,7 +412,7 @@ func TestUnparse(t *testing.T) { name: "call_wrap_default_operators", in: "longVariableA && longVariableB || longVariableC + longVariableD - longVariableE", out: "longVariableA &&\nlongVariableB ||\nlongVariableC + longVariableD - longVariableE", - formattingOptions: []UnparserOption{ + unparserOptions: []UnparserOption{ WrapOnColumn(3), }, }, @@ -432,7 +432,7 @@ func TestUnparse(t *testing.T) { if len(iss.GetErrors()) > 0 { t.Fatalf("parser.Parse(%s) failed: %v", tc.in, iss.ToDisplayString()) } - out, err := Unparse(p.GetExpr(), p.GetSourceInfo(), tc.formattingOptions...) + out, err := Unparse(p.GetExpr(), p.GetSourceInfo(), tc.unparserOptions...) if err != nil { t.Fatalf("Unparse(%s) failed: %v", tc.in, err) @@ -466,10 +466,10 @@ func TestUnparseErrors(t *testing.T) { }, } tests := []struct { - name string - in *exprpb.Expr - err error - formattingOptions []UnparserOption + name string + in *exprpb.Expr + err error + unparserOptions []UnparserOption }{ {name: "empty_expr", in: &exprpb.Expr{}, err: errors.New("unsupported expression")}, { @@ -533,34 +533,34 @@ func TestUnparseErrors(t *testing.T) { err: errors.New("unsupported expression"), }, { - name: "bad_formatting_option_wrap_column_zero", + name: "bad_unparser_option_wrap_column_zero", in: validConstantExpression, - err: errors.New("Invalid formatting option. Wrap column value must be greater than or equal to 1. Got 0 instead"), - formattingOptions: []UnparserOption{ + err: errors.New("Invalid unparser option. Wrap column value must be greater than or equal to 1. Got 0 instead"), + unparserOptions: []UnparserOption{ WrapOnColumn(0), }, }, { - name: "bad_formatting_option_wrap_column_negative", + name: "bad_unparser_option_wrap_column_negative", in: validConstantExpression, - err: errors.New("Invalid formatting option. Wrap column value must be greater than or equal to 1. Got -1 instead"), - formattingOptions: []UnparserOption{ + err: errors.New("Invalid unparser option. Wrap column value must be greater than or equal to 1. Got -1 instead"), + unparserOptions: []UnparserOption{ WrapOnColumn(-1), }, }, { - name: "bad_formatting_option_unsupported_operator", + name: "bad_unparser_option_unsupported_operator", in: validConstantExpression, - err: errors.New("Invalid formatting option. Unsupported operator: bogus"), - formattingOptions: []UnparserOption{ + err: errors.New("Invalid unparser option. Unsupported operator: bogus"), + unparserOptions: []UnparserOption{ WrapOnOperators("bogus"), }, }, { - name: "bad_formatting_option_unary_operator", + name: "bad_unparser_option_unary_operator", in: validConstantExpression, - err: errors.New("Invalid formatting option. Unary operators are unsupported: " + operators.Negate), - formattingOptions: []UnparserOption{ + err: errors.New("Invalid unparser option. Unary operators are unsupported: " + operators.Negate), + unparserOptions: []UnparserOption{ WrapOnOperators(operators.Negate), }, }, @@ -569,7 +569,7 @@ func TestUnparseErrors(t *testing.T) { for _, tst := range tests { tc := tst t.Run(tc.name, func(t *testing.T) { - out, err := Unparse(tc.in, &exprpb.SourceInfo{}, tc.formattingOptions...) + out, err := Unparse(tc.in, &exprpb.SourceInfo{}, tc.unparserOptions...) if err == nil { t.Fatalf("Unparse(%v) got %v, wanted error %v", tc.in, out, tc.err) } From 84b36507bddde80d820bff0b5f8e322c787e77ac Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Mon, 11 Jul 2022 21:04:42 +0000 Subject: [PATCH 7/7] WrapColumn -> WrapOnColumn --- parser/unparser.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/parser/unparser.go b/parser/unparser.go index 48aa3122..64440a94 100644 --- a/parser/unparser.go +++ b/parser/unparser.go @@ -41,7 +41,7 @@ import ( // performing word wrapping on expressions. func Unparse(expr *exprpb.Expr, info *exprpb.SourceInfo, opts ...UnparserOption) (string, error) { unparserOpts := &unparserOption{ - wrapColumn: defaultWrapColumn, + wrapOnColumn: defaultWrapOnColumn, wrapAfterColumnLimit: defaultWrapAfterColumnLimit, operatorsToWrapOn: defaultOperatorsToWrapOn, } @@ -474,7 +474,7 @@ func (un *unparser) writeOperatorWithWrapping(fun string, unmangled string) bool _, wrapOperatorExists := un.options.operatorsToWrapOn[fun] lineLength := un.str.Len() - un.lastWrappedIndex + len(fun) - if wrapOperatorExists && lineLength >= un.options.wrapColumn { + if wrapOperatorExists && lineLength >= un.options.wrapOnColumn { un.lastWrappedIndex = un.str.Len() // wrapAfterColumnLimit flag dictates whether the newline is placed // before or after the operator @@ -502,7 +502,7 @@ func (un *unparser) writeOperatorWithWrapping(fun string, unmangled string) bool // Defined defaults for the unparser options var ( - defaultWrapColumn = 80 + defaultWrapOnColumn = 80 defaultWrapAfterColumnLimit = true defaultOperatorsToWrapOn = map[string]bool{ operators.LogicalAnd: true, @@ -516,7 +516,7 @@ type UnparserOption func(*unparserOption) (*unparserOption, error) // Internal representation of the UnparserOption type type unparserOption struct { - wrapColumn int + wrapOnColumn int operatorsToWrapOn map[string]bool wrapAfterColumnLimit bool } @@ -526,7 +526,7 @@ type unparserOption struct { // // Example usage: // -// Unparse(expr, sourceInfo, WrapColumn(40), WrapOnOperators(Operators.LogicalAnd)) +// Unparse(expr, sourceInfo, WrapOnColumn(40), WrapOnOperators(Operators.LogicalAnd)) // // This will insert a newline immediately after the logical AND operator for the below example input: // @@ -541,7 +541,7 @@ func WrapOnColumn(col int) UnparserOption { if col < 1 { return nil, fmt.Errorf("Invalid unparser option. Wrap column value must be greater than or equal to 1. Got %v instead", col) } - opt.wrapColumn = col + opt.wrapOnColumn = col return opt, nil } } @@ -577,7 +577,7 @@ func WrapOnOperators(symbols ...string) UnparserOption { // // Example usage: // -// Unparse(expr, sourceInfo, WrapColumn(40), WrapOnOperators(Operators.LogicalAnd), WrapAfterColumnLimit(false)) +// Unparse(expr, sourceInfo, WrapOnColumn(40), WrapOnOperators(Operators.LogicalAnd), WrapAfterColumnLimit(false)) // // This will insert a newline immediately before the logical AND operator for the below example input, ensuring // that the length of a line never exceeds the specified column limit: