Closed colinsurprenant closed 8 years ago
@rikwasmus, @alex88, @guyboertje comments/reviews ?
Thank you for your rewrite, I do agree that the logic deserves its own testable function.
However, 2 issues:
Time::at
if the timestamp is actually a float, only being a BigDecimal
(as it return from the parser as one) triggers the problem. It is now testing a condition which wasn't a problem in the first place, and not testing the condition which triggered it.And thank you both for your attention & help :)
@rikwasmus
Could you alter your tests to feed it BigDecimals instead of floats?
errr :stuck_out_tongue: thanks for respectfully reminding me to add the single test case this was all about in the first place! :laughing:
As you suggested and also @alex88 in the code comments, I optimized the coercion for the BigDecimal
case and the rest. My original one-liner was an all-size-fits-all implementation but even if I think the performance impact is really minimal here, the code intent is a lot more clear now and, well, there is less casting and useless object creation done. :heavy_check_mark:
Let me know what you think.
It looks awesome :smile: Except for a rogue timetstamp
in the description of the JSON test (thanks for adding that as well) it gets my vote for shipping ;).
sweet djeezus, I keep on doing that typo :P ... elastic/logstash#4816
LGTM
(I might have used build_event
instead of new_event
, because it does more than create a new event - but I not going to stand on ceremony)
Perfect! will merge and publish. Thanks all! :shipit:
Have a build error that will be fixed by elastic/logstash#4827 - want to merge that first then we will publish after, hopefully today.
elastic/logstash#4827 is merged but discovered another problem with BigDecimal
handling. Fix awaiting reviews for merge in elastic/logstash#4838
published version 2.0.3
this refactors, extends and replaces #32 this will fix elastic/logstash#4565