-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add .aggregate() function to timelion #11556
Conversation
import _ from 'lodash'; | ||
|
||
module.exports = function (points) { | ||
return _.first(points); |
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.
do we really need a wrapper around lodash ? we use it directly everywhere else in our codebase ? (goes for this, min, max, sum, last)
maybe add |
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.
LGTM
@thomasneirynck Did you still want to review this? I'd like to get it merge for 5.5 |
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.
This is a nice addition!
I'd consider inlining the implementations to reduce the footprint.
max: require('./max'), | ||
last: require('./last'), | ||
first: require('./first'), | ||
sum: require('./sum') |
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.
similar comment as @ppisljar. Why not just inline the implementations here?
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.
They're separate because I reuse them in a function I haven't submitted a pull for yet: https://github.com/rashidkpc/timelion-extras/blob/master/functions/orderby/index.js
The purpose of
.aggregate()
is to reduce the set of points in a series to a single, static, value. This is especially useful for things like usingaggregate(first)
for calculating growth since inception of stocks