search

twitter / twitter-server

Twitter-Server defines a template from which services at Twitter are built
http://twitter.github.io/twitter-server/
Apache License 2.0
1.57k stars 264 forks source link

Remove LogFormat trait from TwitterServer #48

Closed crispywalrus closed 7 years ago

crispywalrus commented 8 years ago

Problem

LogFormat bypasses the util Logging and Formatting objects and directly sets the jul Formatter. This means log messages initial are logged in the LogFormat format rather than the user define Formatter format. This simply removes the trait from default TwitterServer without deleting the trait so that users can add it in later if they want its format.

Solution

Remove LogFormat from the stack of traits TwitterServer extends by default. Leaving the trait so it can be used.

Result

Using an override of the defaultFormatter works after the change.

crispywalrus commented 8 years ago

This resolves #47

codecov-io commented 8 years ago

Current coverage is 65.10% (diff: 100%)

Merging #48 into develop will not change coverage

@@            develop        #48   diff @@
==========================================
  Files            63         63          
  Lines          1533       1533          
  Methods        1408       1401     -7   
  Messages          0          0          
  Branches        125        132     +7   
==========================================
  Hits            998        998          
  Misses          535        535          
  Partials          0          0

Powered by Codecov. Last update 562f50a...635b495

cacoco commented 8 years ago

@crispywalrus I'm sorry -- but I don't think this is a workable solution. Simply removing the trait from TwitterServer will cause undue pain and is not backwards compatible. It looks like TwitterServer was not intended to allow for the formatting to be overridden as the LogFormat hardcodes a formatting and is not changeable. A better solution would be to make LogFormat understand the Logging trait and implement it's formatting as the defaultFormatter.

crispywalrus commented 8 years ago

@cacoco Ok, so instead of simply leaving the trait and making it non-default this rewrites it to work with com.twitter.logging classes and to meet the documented criteria of being overridable.

crispywalrus commented 8 years ago

That was my bad, the error was actually due to the lack of the type on defaultFormatter

crispywalrus commented 8 years ago

@mosesn add a premain call to load in place