-
Notifications
You must be signed in to change notification settings - Fork 16
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
Treat block and timestamp buckets equally and separately #319
Conversation
const nextBuckets = await this.getNextBuckets(latest); | ||
const afterNextBuckets = await this.getAfterNextBuckets(latest); | ||
|
||
return currentBuckets.concat(nextBuckets).concat(afterNextBuckets); |
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 line is completely okay, but worth to mention it would be also possible to do:
return currentBuckets.concat(nextBuckets).concat(afterNextBuckets); | |
return return [...currentBuckets, ...nextBuckets, ...afterNextBuckets]; |
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.
New-school vs old-school 👍
|
||
public async stop() { | ||
await Promise.all(this.buckets.map(b => b.stop())); | ||
return; |
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.
No need for return
here. It's implicit.
return; |
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.
It's needed, as I don't want to return Promise<void[]>
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.
Why not have these functions return Promise<boolean>
instead? This would probably need an extra reduce to check all values are true
but it would better than returning void
I think
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.
What should happen if some are not true
?
Pull Request Test Coverage Report for Build 2182
💛 - Coveralls |
Another (hopefully last) attempt to simplify and optimize bucket handling.