-
Notifications
You must be signed in to change notification settings - Fork 120
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
Performance fixes #217
Performance fixes #217
Conversation
src/Senders/CurlSender.php
Outdated
|
||
private function maybeSendMoreBatchRequests($accessToken) | ||
{ | ||
$max = $this->maxBatchRequests-count($this->inflightRequests); |
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.
Style: missing spaces around -
do { | ||
$curlResponse = curl_multi_exec($this->multiHandle, $active); | ||
} while ($curlResponse == CURLM_CALL_MULTI_PERFORM); | ||
while ($active && $curlResponse == CURLM_OK) { |
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 a bit confusing to read this code with 3 while loops. It might be a bit more concise if this was all put into the do/while loop with a switch statement to handle the different responses.
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.
Actually, reading this a bit closer it seems like this should be broken up into different functions.
Looking good! Let's disregard the CodeClimate checks. I removed the webhook so it shouldn't show up anymore. |
When can we get these changes merged and released? |
… of preg_replace completely and get_object_vars
This is ready to be merged, the big change in functionality related to batching is off by default, so if it has bugs (which it may), it is easy to turn off and fix. |
1 similar comment
This is ready to be merged, the big change in functionality related to batching is off by default, so if it has bugs (which it may), it is easy to turn off and fix. |
No description provided.