Parse revenue value #91
Conversation
oakbani
commented
Feb 16, 2018
- float/string values will now be accepted
- Added logs to help debug
|
Can one of the admins verify this patch? |
|
build |
| { | ||
| if (!$logger) { | ||
| $logger = new NoOpLogger(); |
There was a problem hiding this comment.
This shouldn't be necessary because we already perform this logic here: https://github.com/optimizely/php-sdk/blob/master/src/Optimizely/Optimizely.php#L112
That should guarantee that the optimizely instance always has a valid logger
There was a problem hiding this comment.
This saves the effort to check $logger is not null before every log statement as in the method below
There was a problem hiding this comment.
I get it. However, my point is that we should assume here that we always pass in a valid logger because when we invoke getRevenue value, we will always have a valid logger and should just pass it in.
| ->method('log') | ||
| ->with(Logger::DEBUG, "Revenue value couldn't be parsed as an integer."); | ||
|
|
||
| $this->assertNull(EventTagUtils::getRevenueValue(array('revenue' => 0.5), $this->loggerMock)); |
There was a problem hiding this comment.
Why aren't these two parseable?
There was a problem hiding this comment.
The event api says that revenue has to be an integer. This PR accepts an int value even if it is passed as a float or string. Should we also entertain these values? If yes, do we pass it as a float or do we round off ro an int?
There was a problem hiding this comment.
No, you are right. This is good.
|
build |
1 similar comment
|
build |
|
@mikeng13 Please review the updated PR. I realized that no logger was being passed to the method from EventBuilder, so basically no debug messaage logged from event tag utils when needed. Have added a logger to EventBuilder class and pass it on the function call. |
|
build |
|
build |