Wiser coders than myself have come to the conclusion that a `switch`
statement is almost always superior to a statement that includes any
`else if`.
The exceptions that I have found in our codebase are just these two:
* The `if else` is followed by an additional statement before the next
condition (separated by a `;`).
* The whole thing is within a `for` loop and `break` statements are
used. In this case, using `switch` would require tagging the `for`
loop, which probably tips the balance.
Why are `switch` statements more readable?
For one, fewer curly braces. But more importantly, the conditions all
have the same alignment, so the whole thing follows the natural flow
of going down a list of conditions. With `else if`, in contrast, all
conditions but the first are "hidden" behind `} else if `, harder to
spot and (for no good reason) presented differently from the first
condition.
I'm sure the aforemention wise coders can list even more reasons.
In any case, I like it so much that I have found myself recommending
it in code reviews. I would like to make it a habit in our code base,
without making it a hard requirement that we would test on the CI. But
for that, there has to be a role model, so this commit eliminates all
`if else` occurrences, unless it is autogenerated code or fits one of
the exceptions above.
Signed-off-by: beorn7 <beorn@grafana.com>
* 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 <roidelapluie@o11y.eu>
* Fix tests
Signed-off-by: Julien Pivotto <roidelapluie@o11y.eu>
* Implement Pretty() function for AST nodes.
Signed-off-by: Harkishen-Singh <harkishensingh@hotmail.com>
This commit adds .Pretty() for all nodes of PromQL AST.
Each .Pretty() prettifies the node it belongs to, and under
no circustance, the parent or child node is touch/prettified.
Read more in the "Approach" part in `prettier.go`
* Refactor functions between printer.go & prettier.go
Signed-off-by: Harkishen-Singh <harkishensingh@hotmail.com>
This commit removes redundancy between printer.go and prettier.go
by taking out the common code into separate private functions.
* Add more unit tests for Prettier.
Signed-off-by: Harkishen-Singh <harkishensingh@hotmail.com>
* Add support for spliting function calls with 1 arg & unary expressions.
Signed-off-by: Harkishen-Singh <harkishensingh@hotmail.com>
This commit does 2 things:
1. It adds support to split function calls that have 1 arg and exceeds the max_characters_per_line
to multiple lines.
2. Splits Unary expressions that exceed the max_characters_per_line. This is done by formatting the child node
and then removing the prefix indent, which is already applied before the unary operator.
This creates a new `model` directory and moves all data-model related
packages over there:
exemplar labels relabel rulefmt textparse timestamp value
All the others are more or less utilities and have been moved to `util`:
gate logging modetimevfs pool runtime
Signed-off-by: beorn7 <beorn@grafana.com>
* Allow VectorSelector.String() without matchers
Previously this method was panicking because it was trying to allocate a
slice with capacity -1. There's nothing saying that VectorSelector
should have matchers, and it's actually prepared to have zero matcher
strings, so it's worth checking instead of panicking.
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
This commit adds `@ <timestamp>` modifier as per this design doc: https://docs.google.com/document/d/1uSbD3T2beM-iX4-Hp7V074bzBRiRNlqUdcWP6JTDQSs/edit.
An example query:
```
rate(process_cpu_seconds_total[1m])
and
topk(7, rate(process_cpu_seconds_total[1h] @ 1234))
```
which ranks based on last 1h rate and w.r.t. unix timestamp 1234 but actually plots the 1m rate.
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
* Move check for empty VectorSelector to typeChecking
* Move check for twice set metric name to typeChecking
* Make child of MatrixSelector a general Node
* rename checkType to checkAST
* Rename fail to addParseErr
* Remove trailing whitespace
Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>
* *: use latest release of staticcheck
It also fixes a couple of things in the code flagged by the additional
checks.
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
* Use official release of staticcheck
Also run 'go list' before staticcheck to avoid failures when downloading packages.
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
When converting `AlertStmt` to a string, the alert rule labels were
printed as `ANNOTATIONS` instead of the annotations themselves.
Fix and add a test to catch future regressions.
Currently the printer doesn't print the annotations of an `*AlertStmt`
declaration. I've added a test case as well, which fails for the current
master.
Since rule evaluations work via String(), this fixes evaluation of
rules containing GROUP_x modifiers without labels. This change is the
minimal bugfix (so that we can release a fixed version without
risk). It does not intend to implement any additional features (like
allowing `GROUP_LEFT()` or `ON()` or even `ON` - see discussion in
https://github.com/prometheus/prometheus/issues/1597 ).
This has the advantage that the user doesn't need
to list all labels they want to keep (as with "by")
but without having to worry about inconsistent labels
as when there's only one time series (as with "keeping_common").
Almost all aggregation should use this rather than the existing
two options as it's much less error prone and easier to maintain
due to not having to always add in "job" plus whatever other common
job-level labels you have like "region".
It's actually happening in several places (and for flags, we use the
standard Go time.Duration...). This at least reduces all our
home-grown parsing to one place (in model).
`keep_common` is more in line with the function name
`drop_common_labels()` terminology-wise, and also more in line with
`group_left`/`group_right` (no `...ing` verb suffix).
We could also go the full way and call it `keep_common_labels`. That
would have the benefit of being even more consistent with the function
`drop_common_labels()` and would be more explanatory, but it also seems
quite long.
beorn7