Skip to content

feat: Add batch processing for eachAsync#9902

Merged
vkarpov15 merged 1 commit intoAutomattic:5.12from
khaledosama999:gh-9861
Feb 15, 2021
Merged

feat: Add batch processing for eachAsync#9902
vkarpov15 merged 1 commit intoAutomattic:5.12from
khaledosama999:gh-9861

Conversation

@khaledosama999
Copy link
Copy Markdown
Contributor

@khaledosama999 khaledosama999 commented Feb 5, 2021

Resolves #9861

@khaledosama999 khaledosama999 changed the base branch from master to 6.0 February 5, 2021 18:53
@khaledosama999
Copy link
Copy Markdown
Contributor Author

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.

@khaledosama999 khaledosama999 changed the title Feature: Add batch processing for eachAsync feat: Add batch processing for eachAsync Feb 5, 2021
@khaledosama999
Copy link
Copy Markdown
Contributor Author

I can rebase the feature on the master branch and change the base of the PR to master if you want

@khaledosama999
Copy link
Copy Markdown
Contributor Author

@vkarpov15

Copy link
Copy Markdown

@miladezzat miladezzat left a 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) {
Copy link
Copy Markdown
Collaborator

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.

Copy link
Copy Markdown
Collaborator

@vkarpov15 vkarpov15 left a 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?

@khaledosama999
Copy link
Copy Markdown
Contributor Author

@vkarpov15 No fine by me, like I said if you want me to rebase the feature on another branch that's fine.

@vkarpov15
Copy link
Copy Markdown
Collaborator

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.

@khaledosama999
Copy link
Copy Markdown
Contributor Author

khaledosama999 commented Feb 12, 2021

No problem, give me a few mins

@khaledosama999 khaledosama999 changed the base branch from 6.0 to 5.12 February 12, 2021 22:33
@khaledosama999
Copy link
Copy Markdown
Contributor Author

@vkarpov15 Done, no idea why Travis CI failed though.

Copy link
Copy Markdown
Collaborator

@vkarpov15 vkarpov15 left a 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

This was referenced Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants