-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
feat: Add batch processing for eachAsync #9902
Conversation
Not the proudest code I've written, but it does the job with minimal changes but honestly, I would prefer for it to be a standalone method. |
I can rebase the feature on the master branch and change the base of the PR to master if you want |
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.
Good Job Mr @khaledosama999 , I think it will be a good feature for me
} | ||
|
||
// If the current documents size is less than the provided patch size don't process the documents yet | ||
if (batchSize && documentsBatch.length !== batchSize) { |
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.
I'd do documentsBatch.length < batchSize
just in case, but I don't see that being a blocker 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.
LGTM 👍 I'll merge this into the 5.12 branch and release this with 5.12.0 unless you have any objections?
@vkarpov15 No fine by me, like I said if you want me to rebase the feature on another branch that's fine. |
My mistake, I didn't notice this was against the 6.0 branch. Would it be possible for you to rebase this against the 5.12 branch? 6.0 is a major backwards breaking release that we plan on shipping in early 2021, 5.12 is a feature release we're planning on shipping in Feb '21. |
No problem, give me a few mins |
37d39e2
to
007fb69
Compare
@vkarpov15 Done, no idea why Travis CI failed though. |
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.
Our travis config is unfortunately long out of date and we haven't taken the time to fix it yet. The rest of the tests passed, so LGTM
Resolves #9861