Skip to content

Merged in `capistrano_colors` gem, and renamed to 'log_formatters' #283

Merged
merged 1 commit into from Oct 24, 2012

4 participants

@ndbroadbent

I can't imagine why anyone would want to use capistrano without the capistrano_colors formatting. We've been adding it to all of our Gemfiles for years now, and I strongly feel that it should be a default feature.

capistrano_colors also does more than just colors. It can change the log style (underline, etc.), and can even prepend text or timestamps to the line. That's why I decided to call it 'log_formatters' instead.

Anyone can disable the new log formatting by adding disable_log_formatters to their deploy.rb. This will turn off the formatters and produce the same output as before. Log formatting will be automatically disabled if the output device is a file.

I've prepared a Log Formatters wiki page to add to the wiki index.

If this gets merged, I will be available to fix any bugs or issues.

@ndbroadbent ndbroadbent referenced this pull request in stjernstrom/capistrano_colors Oct 1, 2012
Closed

Pull request opened to merge into capistrano core #12

@ndbroadbent

I've just added a complete set of tests for this feature at test/logger_formatting_test.rb, which were missing from the capistrano_colors gem.

@jarthod
jarthod commented Oct 5, 2012

Agreed, this should be the default output !

@kirs
Capistrano member
kirs commented Oct 22, 2012

❤️

@carsomyr

@ndbroadbent Could you add TTY detection?

@ndbroadbent ndbroadbent commented on an outdated diff Oct 23, 2012
lib/capistrano/logger.rb
@@ -29,6 +70,46 @@ def close
def log(level, message, line_prefix=nil)
if level <= self.level
+ # Don't format output if logger is outputting to a file,
@ndbroadbent
ndbroadbent added a note Oct 23, 2012

@carsomyr - This is close to tty detection. It will only format the logs if they are being printed to stdout. No formatting will occur if they are being saved to a file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carsomyr

@ndbroadbent Why not go the whole way and use STDOUT.tty?? Also, could you change the @@field instances to class-instance variables?

@ndbroadbent

Not sure what you mean about using STDOUT.tty? And which @@field variables are you talking about?

@ndbroadbent

@@formatters needs to be a class-level instance variable in order to provide a DSL for adding custom formatters

@carsomyr

@ndbroadbent I was wondering if it would be better to use TTY detection on IO objects. As for class-level instance variables, see this post.

@ndbroadbent

Ah, sorry, understood. I have changed those to class-instance variables

@ndbroadbent

how does 1f49d671123d36d7a174b2a7e10af881c8f3fff9 look, RE: tty ?

@carsomyr

@ndbroadbent re: the TTY check, I don't think you have to check if it's a file anymore (files aren't TTYs, AFAIK). re: the class-level instance variables, looks good. Squash all the commits? I wanna try it locally first.

@ndbroadbent

Great point, I've changed it to just do device.tty?, and have squashed all commits

@carsomyr

@ndbroadbent Some final nitpicky observations:

  1. The comment in line 90 should read differently.
  2. You may want to change self.class.sorted_formatters to Logger.sorted_formatters.
  3. Are you able to replace the formatters= method with something else? It seems to exist solely for the purpose of unit tests.
@ndbroadbent

1) Good point!
2) Done!
3) Moved into a class_eval in the test, since I think you're right, users should only be using the add_formatter method in config/deploy.rb.

Thanks for the feedback, you're definitely raising some valid points!

@carsomyr carsomyr merged commit e909441 into capistrano:master Oct 24, 2012

1 check passed

Details default The Travis build passed
@carsomyr

@ndbroadbent It's merged! Your next mission, should you choose to accept it... Well, you know which PR I am referring to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.