add macros for raw values of interval, from, to #98
Conversation
|
This pr fixes #103 and https://github.com/grafana/support-escalations/issues/866 |
pkg/timestream/macros_test.go
Outdated
| @@ -93,4 +93,69 @@ func TestInterpolate(t *testing.T) { | |||
| t.Fatalf("Result above tolerated precision %d : %d, %d", precision, numtext, expect) | |||
| } | |||
| }) | |||
|
|
|||
| t.Run("using timeFrom", func(t *testing.T) { | |||
| sqltxt := `$__time_from_raw_ms` | |||
There was a problem hiding this comment.
Repeating string value in two different files seems a little delicate / prone to potential maintainability issues down the road.
I'm not as familiar with Go / Go conventions, but in naming a const is the naming convention camelCase vs. CONST_VAR (similar to JS / TS?). Making naming of const very clear may help some (starting on line 13 of macros.go) Also, are we able to export individual variables from a file? If so we could export the const string values from macros.go to use them with more confidence in this testing file.
Disclaimer: This may be a premature optimization / me not understanding Go conventions :)
There was a problem hiding this comment.
The Go standard library uses camel case for consts, since capital = export. They're in the same package, so they actually wouldn't need to be exported to be used in this test. However, I somewhat disagree with using the exact same string object in the code and the test, since I don't think that tests much. (See other comment about using a longer sqltxt string)
pkg/timestream/macros_test.go
Outdated
| @@ -93,4 +93,69 @@ func TestInterpolate(t *testing.T) { | |||
| t.Fatalf("Result above tolerated precision %d : %d, %d", precision, numtext, expect) | |||
| } | |||
| }) | |||
|
|
|||
| t.Run("using timeFrom", func(t *testing.T) { | |||
| sqltxt := `$__time_from_raw_ms` | |||
There was a problem hiding this comment.
The Go standard library uses camel case for consts, since capital = export. They're in the same package, so they actually wouldn't need to be exported to be used in this test. However, I somewhat disagree with using the exact same string object in the code and the test, since I don't think that tests much. (See other comment about using a longer sqltxt string)
|
sorry for the delay in getting to this. Hope it looks a little bit better now |
There was a problem hiding this comment.
Hi @DarrenStack ,
One last thing, but it looks really good!
|
Sorry I missed the review re-request! It looks good now and thanks for the PR! Could you fix the merge conflict before I merge it? |
|
No worries! I slightly updated the README again to make the difference between the macros a bit clearer. |
Hi there,
When using the Grafana plugin, we sometimes have to do some string replacement and casting on the current interval and timeFilter macros in order to use the raw values for calculations.
It would be nicer for us if we could have macros to access those values directly like this PR allows.
Happy to change anything that doesn't fit with your macro standards.