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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[query] Implemented movingSum, movingMax, movingMin (graphite functions) #2570
[query] Implemented movingSum, movingMax, movingMin (graphite functions) #2570
Changes from 24 commits
4e901d7
f42685a
f0df4ce
bef6377
8ce6dc3
30ef946
cf9288f
50b0ae9
1ebe629
ebaafce
2a0643c
53222b9
8c2b6bf
01792e7
27f5d53
b9e6a92
91b2778
c024c85
a2698c2
7c1129d
9c070b8
a0807a4
9877de0
86e9b6a
3dbc68e
2124c31
3a201d6
1847586
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Go prefers to limit indentation wherever possible (since it's relative to complexity of the statement). Can simplify loop by following's advice of Effective Go "In the Go libraries, you'll find that when an if statement doesn't flow into the next statement—that is, the body ends in break, continue, goto, or return—the unnecessary else is omitted.":
https://golang.org/doc/effective_go.html#control-structures
e.g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Go prefers to limit indentation wherever possible (since it's relative to complexity of the statement). Can simplify loop by following's advice of Effective Go "In the Go libraries, you'll find that when an if statement doesn't flow into the next statement—that is, the body ends in break, continue, goto, or return—the unnecessary else is omitted.":
https://golang.org/doc/effective_go.html#control-structures
e.g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some explicit tests to make sure this case is handled ok? Would add a test case to
engine_test.go
and insideTestExecute
to make sure a binary transform correctly is handled if anil
binary context shifter is returned (i.e. send input values of zero).I just spent 10-20mins finding if it was or not and not 100% confident about it. I see this which implies it should be:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
So you said you want me to add "a test case to
engine_test.go
and insideTestExecute
"i added the test case to
engine_test.go
but i dont quite understand your wording on the second part. were you saying that you also wanted me to modify
TestExecute
to test whether or not a binary transform is correctly handled if anil
binary context shifter is returned? why would I need to test for that in two places?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved offline. Rob said this test was enough.