Hi Ceki, Thanks for your quick follow up. My comments will be below as well.
On Sun, Aug 11, 2019 at 10:26 PM Ceki Gülcü <c...@qos.ch> wrote: > > Hi Wessel, > > Comments below. > > On 09/08/2019 01:09, Wessel van Norel wrote: > > I've not yet created the test cases for the EventRecordingLogger, I'll > > do that in case it's indeed unintentional that it's loosing the > > Throwables. I've added two tests for the changes I made to the > > MessageFormatter. While I was at it I also applied some improvements > > IntelliJ suggested. > > Very good. In general, it is preferable if the more meaningful changes > are separate from more cosmetic ones even if both are welcome. > Sure no problem. Do you want a separate JIRA Ticket for it? > You can see my changes on my fork: > > > > > https://github.com/qos-ch/slf4j/compare/master...delgurth:eventrecordinglogger?expand=1 > > I have made some comments there. > >From those remarks: > Instead, an analogous check can be placed in recordEvent() avoiding repetition. Sounds like a good plan. But I'll have to think about how to deal with the 2 object argument calls. Since if just make an two element array out of it and then have to make it into a one element array and a throwable that's a bit wasteful, isn't it? > In any case, I think the unit test as well the change to the > org.slf4j.event.EventRecodingLogger.recordEvent method are warranted. I > propose to discuss changes to MessageFormatter class separately. > Ok. I'll create a Jira ticket about the MessageFormatter. The stacktrace as last parameter is great for users of slf4j, but for the API implementation it's a bit of a pain. Do you have a performance benchmark around slf4j or specific the messageformatter (since the javadoc says the slf4j version is 10 times faster then the version provided by java itself)? I found a recently created SLF4J JMH project: https://github.com/wsargent/slf4j-benchmark but I'm not sure if that project is doing the proper benchmarking. Also can you please create a Jira ticket describing the bug in > EventRecodingLogger.recordEvent? > Will do. > Many thanks, > > -- > Ceki > You're most welcome. Kind regards, Wessel _______________________________________________ > slf4j-dev mailing list > slf4j-dev@qos.ch > http://mailman.qos.ch/mailman/listinfo/slf4j-dev
_______________________________________________ slf4j-dev mailing list slf4j-dev@qos.ch http://mailman.qos.ch/mailman/listinfo/slf4j-dev
