Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposed Logger rework to fix issue 728 #736

Merged
merged 14 commits into from Jan 23, 2012
Merged

Conversation

skade
Copy link
Contributor

@skade skade commented Dec 23, 2011

Hi,

this evolved into a larger patch then intended, but hopefully satisfies all parties. I tried to refactor the Padrino Logger so that all the fancy stuff @DAddYE built is still working and - as an added bonus - can be added to other loggers as well. This works by splitting the logger implementation into 2 modules for proper extensions (like the #bench method for benchmarks and the #devel log level) and one for colorization (as some loggers, like syslog, cannot handle colors). Extensions are always added to any logger that is passed to #logger=, Colorization is only added on the users request. Example:

Padrino.logger = Lumberjack::Logger.new
Padrino.logger.colorize!
Padrino.logger.debug("Horay, a colorized Lumberjack logger!")

Also, I took the liberty of adding all those modules to a Padrino::Logging submodule, while keeping Padrino::Logger as is.

This pull request also adds tests using Lumberjack and the ruby stdlib Logger as examples.

Merry Christmas, by the way.

Best regards,
Florian

Some other loggers do not have a logstream and do not fit into
'rack.errors'
Colorize allows to add Padrino message colorization to any logger
that is annotated with LoggingExtensions.
colorize now returns the colorized string, not the original.
Allows to annotate other loggers with padrino fancyness
This ensures that the logger actually supports colorization
format_message is used by ruby stdlib logger
@nesquena
Copy link
Member

Looks great, thanks for putting this together. @DAddYE can you review this and merge it since you've played with logging the most?

@DAddYE
Copy link
Member

DAddYE commented Dec 23, 2011

Sure @nesquena Im doing that now, many thanks to @skade !

@skade
Copy link
Contributor Author

skade commented Jan 23, 2012

How about this one? I would really like to deploy Lumberjack in production.

@nesquena
Copy link
Member

I agree @DAddYE Can you review and merge this before the release?

@DAddYE
Copy link
Member

DAddYE commented Jan 23, 2012

@nesquena sure, My apologies @skade for keeping you waiting.

@skade
Copy link
Contributor Author

skade commented Jan 23, 2012

No reason to apologize :). But skipping one revision would be more work for everyone.

DAddYE added a commit that referenced this pull request Jan 23, 2012
Proposed Logger rework to fix issue 728
@DAddYE DAddYE merged commit 4d28a66 into padrino:master Jan 23, 2012
@DAddYE
Copy link
Member

DAddYE commented Jan 23, 2012

Thanks @skade I will re-couple into a single module to keep things more readable, btw awesome refactoring! Thanks so so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants