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

Conversation

Projects
None yet
3 participants
@skade
Member

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

skade added some commits Dec 20, 2011

Don't rely on Padrino::Logger#log
Some other loggers do not have a logstream and do not fit into
'rack.errors'
Delegate colorization to logger
This ensures that the logger actually supports colorization
Add #colorize!
Colorize allows to add Padrino message colorization to any logger
that is annotated with LoggingExtensions.
Make #colorize work correctly
colorize now returns the colorized string, not the original.
Factor out padrino logger extensions into modules
Allows to annotate other loggers with padrino fancyness
Use #format instead of #format_message
format_message is used by ruby stdlib logger
@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Dec 23, 2011

Member

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

Member

nesquena commented Dec 23, 2011

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

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Dec 23, 2011

Member

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

Member

DAddYE commented Dec 23, 2011

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

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Jan 23, 2012

Member

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

Member

skade commented Jan 23, 2012

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

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jan 23, 2012

Member

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

Member

nesquena commented Jan 23, 2012

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

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Jan 23, 2012

Member

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

Member

DAddYE commented Jan 23, 2012

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

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Jan 23, 2012

Member

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

Member

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

Merge pull request #736 from skade/issue728
Proposed Logger rework to fix issue 728

@DAddYE DAddYE merged commit 4d28a66 into padrino:master Jan 23, 2012

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Jan 23, 2012

Member

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

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