Handle default arguments and argument number to avoid errors #121
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit adds the files that are specific to Grafana's fork that are not intended to be included in the upstream. This files are already present in the commit history (they were deleted), but this is an attempt of a fresh start on top of the upstream's `main`. At the moment, this should the only commit we expect to skip when sending our changes.
* Fix exponentialMovingAverage panic The panic was happening when the window size was greater than the length of the values. Now if that happens no panics occur and the average is returned. This matches graphiteweb. * Add support for Graphite web toLowerCase function * Add tests for toLowerCase function * Update glue.go * Update COMPATIBILITY.md to show support of toLowerCase * Add support for Graphite web toUpperCase function * Add tests for toUpperCase function * Update glue.go * Update COMPATIBILITY.md to include toUpperCase function * Add support for Graphite web sumSeriesLists function * Add tests for sumSeriesLists * Update glue.go * Update COMPATIBILITY.md to include sumSeriesLists * Fix problems that I accidentally introduced while trying to resolve merge conflict * Another attempt to fix previous problem Apparantly there is a directory that gen.go wasn't expecting and `go generate` in expr/functions produced `glue.go` file that didn't compile fix both generator and glue * Adds Drone and Github workflow for Grafana's fork This commit adds the files that are specific to Grafana's fork that are not intended to be included in the upstream. This files are already present in the commit history (they were deleted), but this is an attempt of a fresh start on top of the upstream's `main`. At the moment, this should the only commit we expect to skip when sending our changes. Co-authored-by: Nicolás Pazos <npazosmendez@gmail.com> Co-authored-by: Vladimir Smirnov <civiloid@google.com>
This comment has been minimized.
This comment has been minimized.
ywwg
reviewed
Sep 20, 2022
| if err != nil { | ||
| return nil, err | ||
| } | ||
| callback, err := e.GetStringArgDefault(1, "average") |
There was a problem hiding this comment.
what is the story behind this part of the change?
There was a problem hiding this comment.
I just replaced the switch statement with functions that will set the default argument, but I can see that there could be a bug here, so I'll revert that change
Comment on lines
73
to
74
| n = 1 | ||
| consolidation, err = e.GetStringArg(1) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
| @@ -28,7 +28,7 @@ func New(configFile string) []interfaces.FunctionMetadata { | |||
| return res | |||
| } | |||
|
|
|||
| // squareRoot(seriesList) | |||
| // randomWalk(seriesList, step=60) | |||
There was a problem hiding this comment.
nit: this hould be randomWalk(name, step=60)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
carrieedwards
force-pushed
the
cedwards/fix-argument-handling
branch
from
e110194
to
efbb16c
Compare
|
Go coverage report: Click to expand.
Go lint report: No issues found. 😎 |
|
Closing in favor of go-graphite#712 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds error handling to function arguments. If fewer arguments than the minimum required by that function are passed in, then a missing argument error is now returned. Additionally, some functions with default values for arguments were missing the default, so those have been updated. This will prevented unexpected out-of-index panics and other errors from occurring.