Conversation
NoelDavies
left a comment
NoelDavies
left a comment
There was a problem hiding this comment.
I've added a few comments, but so far it looks good. I think it'd be best to keep everything strictly typed to avoid any issues that may arise.
Bump to version 1.0.0 (API is the same, but requires php 7.1) Approved-by: Andre Ferraz <andre.ferraz@endclothing.com> Approved-by: DuShaun Alderson-Claeys <dushaun.alderson-claeys@endclothing.com> Approved-by: Robin Shimield <robin.shimield@endclothing.com>
|
@danvirsen can you please update your branch to be up to date with master? |
danvirsen
force-pushed
the
feature/summary-collector
branch
from
df60bc3 to
2c826b9
Compare
|
Sorry for the delay. I had email notifications turned off and didn't notice the responses. |
|
@danvirsen tests are missing |
|
Yes, I'm aware. I will try to take some time to write tests, but I'm not sure when that will be. |
| $summaries[] = $summary; | ||
| } | ||
| return $summaries; | ||
| } |
There was a problem hiding this comment.
There are a LOT of nested loops in here, can we reduce these to improve performance?
| $quantiles = self::getDefaultQuantiles(); | ||
| } | ||
|
|
||
| if (0 === count($quantiles)) { |
There was a problem hiding this comment.
We need to flip these yoda statements to be consistent with the existing code base
|
Any updates on this? |
|
Hi @marcospassos, I'm afraid I no longer work at EndClothing. So one of the developers there should be maintaining this soon. |
|
@NoelDavies Thank you for replying! |
4 participants
This is basically buffcode's code slightly tweaked to make it mergeable with this fork.
There are no tests and the only storage adapter that is implemented is Redis, so I wouldn't say it's ready for a merge just yet.
I'll try to get some time to take a look at tests and the other storage adapters, but I'm not sure when that might be. If someone else would like to take a look at it, they're more than welcome. 🙂