From d41e5a558294666ba572648b454b5f7a5f548573 Mon Sep 17 00:00:00 2001 From: Julien Pivotto Date: Fri, 15 Jul 2022 00:09:56 +0200 Subject: [PATCH] Prettifier: Add spaces with non-callable keywords (#11005) * Prettifier: Add spaces with non-callable keywords I prefer to have a difference between, on one side: functions calls, end(), start(), and on the other side with, without, ignoring, by and group_rrigt, group_left. The reasoning is that the former ones are not calls, while other are functions. Additionally, it matches the examples in our documentation. Signed-off-by: Julien Pivotto * Fix tests Signed-off-by: Julien Pivotto --- promql/parser/prettier_test.go | 62 ++++++++++++++++++---------------- promql/parser/printer.go | 8 ++--- promql/parser/printer_test.go | 29 ++++++++++------ 3 files changed, 56 insertions(+), 43 deletions(-) diff --git a/promql/parser/prettier_test.go b/promql/parser/prettier_test.go index 7762e7448a..16f4906f62 100644 --- a/promql/parser/prettier_test.go +++ b/promql/parser/prettier_test.go @@ -37,25 +37,25 @@ func TestAggregateExprPretty(t *testing.T) { }, { in: `sum without(job,foo) (task:errors:rate10s{job="s"})`, - out: `sum without(job, foo) ( + out: `sum without (job, foo) ( task:errors:rate10s{job="s"} )`, }, { in: `sum(task:errors:rate10s{job="s"}) without(job,foo)`, - out: `sum without(job, foo) ( + out: `sum without (job, foo) ( task:errors:rate10s{job="s"} )`, }, { in: `sum by(job,foo) (task:errors:rate10s{job="s"})`, - out: `sum by(job, foo) ( + out: `sum by (job, foo) ( task:errors:rate10s{job="s"} )`, }, { in: `sum (task:errors:rate10s{job="s"}) by(job,foo)`, - out: `sum by(job, foo) ( + out: `sum by (job, foo) ( task:errors:rate10s{job="s"} )`, }, @@ -68,17 +68,17 @@ func TestAggregateExprPretty(t *testing.T) { }, { in: `sum by(job,foo) (sum by(job,foo) (task:errors:rate10s{job="s"}))`, - out: `sum by(job, foo) ( - sum by(job, foo) ( + out: `sum by (job, foo) ( + sum by (job, foo) ( task:errors:rate10s{job="s"} ) )`, }, { in: `sum by(job,foo) (sum by(job,foo) (sum by(job,foo) (task:errors:rate10s{job="s"})))`, - out: `sum by(job, foo) ( - sum by(job, foo) ( - sum by(job, foo) ( + out: `sum by (job, foo) ( + sum by (job, foo) ( + sum by (job, foo) ( task:errors:rate10s{job="s"} ) ) @@ -87,8 +87,8 @@ func TestAggregateExprPretty(t *testing.T) { { in: `sum by(job,foo) (sum by(job,foo) (task:errors:rate10s{job="s"}))`, - out: `sum by(job, foo) ( - sum by(job, foo) ( + out: `sum by (job, foo) ( + sum by (job, foo) ( task:errors:rate10s{job="s"} ) )`, @@ -96,8 +96,8 @@ func TestAggregateExprPretty(t *testing.T) { { in: `sum by(job,foo) (sum(task:errors:rate10s{job="s"}) without(job,foo))`, - out: `sum by(job, foo) ( - sum without(job, foo) ( + out: `sum by (job, foo) ( + sum without (job, foo) ( task:errors:rate10s{job="s"} ) )`, @@ -106,8 +106,8 @@ func TestAggregateExprPretty(t *testing.T) { in: `sum by(job,foo) # Comment 1. (sum by(job,foo) ( # Comment 2. task:errors:rate10s{job="s"}))`, - out: `sum by(job, foo) ( - sum by(job, foo) ( + out: `sum by (job, foo) ( + sum by (job, foo) ( task:errors:rate10s{job="s"} ) )`, @@ -139,7 +139,7 @@ func TestBinaryExprPretty(t *testing.T) { { in: `a + ignoring(job) b`, out: ` a -+ ignoring(job) ++ ignoring (job) b`, }, { @@ -175,19 +175,21 @@ func TestBinaryExprPretty(t *testing.T) { { in: `foo_1 + ignoring(foo) foo_2 + ignoring(job) group_left foo_3 + on(instance) group_right foo_4`, out: ` foo_1 - + ignoring(foo) + + ignoring (foo) foo_2 - + ignoring(job) group_left() + + ignoring (job) group_left () foo_3 -+ on(instance) group_right() ++ on (instance) group_right () foo_4`, }, } for _, test := range inputs { - expr, err := ParseExpr(test.in) - require.NoError(t, err) + t.Run(test.in, func(t *testing.T) { + expr, err := ParseExpr(test.in) + require.NoError(t, err) - require.Equal(t, test.out, Prettify(expr)) + require.Equal(t, test.out, Prettify(expr)) + }) } } @@ -560,7 +562,7 @@ or }, { in: `min by (job, integration) (rate(alertmanager_notifications_failed_total{job="alertmanager", integration=~".*"}[5m]) / rate(alertmanager_notifications_total{job="alertmanager", integration="~.*"}[5m])) > 0.01`, - out: ` min by(job, integration) ( + out: ` min by (job, integration) ( rate( alertmanager_notifications_failed_total{integration=~".*",job="alertmanager"}[5m] ) @@ -575,7 +577,7 @@ or { in: `(count by (job) (changes(process_start_time_seconds{job="alertmanager"}[10m]) > 4) / count by (job) (up{job="alertmanager"})) >= 0.5`, out: ` ( - count by(job) ( + count by (job) ( changes( process_start_time_seconds{job="alertmanager"}[10m] ) @@ -583,7 +585,7 @@ or 4 ) / - count by(job) ( + count by (job) ( up{job="alertmanager"} ) ) @@ -630,7 +632,7 @@ func TestUnaryPretty(t *testing.T) { in: `-histogram_quantile(0.99, sum by (le) (rate(foo[1m])))`, out: `-histogram_quantile( 0.99, - sum by(le) ( + sum by (le) ( rate( foo[1m] ) @@ -659,8 +661,10 @@ func TestUnaryPretty(t *testing.T) { }, } for _, test := range inputs { - expr, err := ParseExpr(test.in) - require.NoError(t, err) - require.Equal(t, test.out, Prettify(expr)) + t.Run(test.in, func(t *testing.T) { + expr, err := ParseExpr(test.in) + require.NoError(t, err) + require.Equal(t, test.out, Prettify(expr)) + }) } } diff --git a/promql/parser/printer.go b/promql/parser/printer.go index f053c167f7..1f15eeef33 100644 --- a/promql/parser/printer.go +++ b/promql/parser/printer.go @@ -77,9 +77,9 @@ func (node *AggregateExpr) getAggOpStr() string { switch { case node.Without: - aggrString += fmt.Sprintf(" without(%s) ", strings.Join(node.Grouping, ", ")) + aggrString += fmt.Sprintf(" without (%s) ", strings.Join(node.Grouping, ", ")) case len(node.Grouping) > 0: - aggrString += fmt.Sprintf(" by(%s) ", strings.Join(node.Grouping, ", ")) + aggrString += fmt.Sprintf(" by (%s) ", strings.Join(node.Grouping, ", ")) } return aggrString @@ -103,14 +103,14 @@ func (node *BinaryExpr) getMatchingStr() string { if vm.On { vmTag = "on" } - matching = fmt.Sprintf(" %s(%s)", vmTag, strings.Join(vm.MatchingLabels, ", ")) + matching = fmt.Sprintf(" %s (%s)", vmTag, strings.Join(vm.MatchingLabels, ", ")) if vm.Card == CardManyToOne || vm.Card == CardOneToMany { vmCard := "right" if vm.Card == CardManyToOne { vmCard = "left" } - matching += fmt.Sprintf(" group_%s(%s)", vmCard, strings.Join(vm.Include, ", ")) + matching += fmt.Sprintf(" group_%s (%s)", vmCard, strings.Join(vm.Include, ", ")) } } return matching diff --git a/promql/parser/printer_test.go b/promql/parser/printer_test.go index caaff7e469..a044b6969c 100644 --- a/promql/parser/printer_test.go +++ b/promql/parser/printer_test.go @@ -33,13 +33,16 @@ func TestExprString(t *testing.T) { out: `sum(task:errors:rate10s{job="s"})`, }, { - in: `sum by(code) (task:errors:rate10s{job="s"})`, + in: `sum by(code) (task:errors:rate10s{job="s"})`, + out: `sum by (code) (task:errors:rate10s{job="s"})`, }, { - in: `sum without() (task:errors:rate10s{job="s"})`, + in: `sum without() (task:errors:rate10s{job="s"})`, + out: `sum without () (task:errors:rate10s{job="s"})`, }, { - in: `sum without(instance) (task:errors:rate10s{job="s"})`, + in: `sum without(instance) (task:errors:rate10s{job="s"})`, + out: `sum without (instance) (task:errors:rate10s{job="s"})`, }, { in: `topk(5, task:errors:rate10s{job="s"})`, @@ -48,26 +51,32 @@ func TestExprString(t *testing.T) { in: `count_values("value", task:errors:rate10s{job="s"})`, }, { - in: `a - on() c`, + in: `a - on() c`, + out: `a - on () c`, }, { - in: `a - on(b) c`, + in: `a - on(b) c`, + out: `a - on (b) c`, }, { - in: `a - on(b) group_left(x) c`, + in: `a - on(b) group_left(x) c`, + out: `a - on (b) group_left (x) c`, }, { - in: `a - on(b) group_left(x, y) c`, + in: `a - on(b) group_left(x, y) c`, + out: `a - on (b) group_left (x, y) c`, }, { in: `a - on(b) group_left c`, - out: `a - on(b) group_left() c`, + out: `a - on (b) group_left () c`, }, { - in: `a - on(b) group_left() (c)`, + in: `a - on(b) group_left() (c)`, + out: `a - on (b) group_left () (c)`, }, { - in: `a - ignoring(b) c`, + in: `a - ignoring(b) c`, + out: `a - ignoring (b) c`, }, { in: `a - ignoring() c`,