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

ability to turn off ansi color in passenger-memory-status output #487

Closed
FooBarWidget opened this Issue May 29, 2014 · 2 comments

Comments

Projects
None yet
2 participants
@FooBarWidget
Member

FooBarWidget commented May 29, 2014

The command line tools like passenger-status should not output in color when stdout is not a TTY. They should also have a command line option for disabling color output.

This issue has been imported from Google Code: http://code.google.com/p/phusion-passenger/issues/detail?id=387

@catchmrbharath

This comment has been minimized.

Show comment
Hide comment
@catchmrbharath

catchmrbharath Jul 26, 2014

Can I work on this? Or will you merge the pull request by @qmx in #14 after which I can add a environment variable?

catchmrbharath commented Jul 26, 2014

Can I work on this? Or will you merge the pull request by @qmx in #14 after which I can add a environment variable?

@FooBarWidget

This comment has been minimized.

Show comment
Hide comment
@FooBarWidget

FooBarWidget Jul 26, 2014

Member

Sure, go ahead. :) There are a number of issues with #14 which is why I didn't merge it:

  • It sets the color constants based on whether STDOUT.tty?. But that's not necessarily the right thing to do. Whether color codes should be enabled depends on the context. Sometimes, checking STDOUT.tty? is the right thing, sometimes not.

    I prefer if the AnsiColors module is refactored into a class. This class would have color methods which either return an ANSI color code, or nothing. The class's constructor would, be default, check whether STDOUT.tty? to determine whether to return ANSI color codes or not, but it should accept a parameter to allow the reader to override the behavior.

    Something like this:

    @colors = Utils::AnsiColors.new  # Should accept true, false, or :auto. Default is :auto.
    puts "#{colors.red}An error occurred.#{colors.reset}"
    # Same thing:
    puts colors.red { "An error occurred." }
    
  • Inconsistent indenting. We use tabs, not two-spaces.

Member

FooBarWidget commented Jul 26, 2014

Sure, go ahead. :) There are a number of issues with #14 which is why I didn't merge it:

  • It sets the color constants based on whether STDOUT.tty?. But that's not necessarily the right thing to do. Whether color codes should be enabled depends on the context. Sometimes, checking STDOUT.tty? is the right thing, sometimes not.

    I prefer if the AnsiColors module is refactored into a class. This class would have color methods which either return an ANSI color code, or nothing. The class's constructor would, be default, check whether STDOUT.tty? to determine whether to return ANSI color codes or not, but it should accept a parameter to allow the reader to override the behavior.

    Something like this:

    @colors = Utils::AnsiColors.new  # Should accept true, false, or :auto. Default is :auto.
    puts "#{colors.red}An error occurred.#{colors.reset}"
    # Same thing:
    puts colors.red { "An error occurred." }
    
  • Inconsistent indenting. We use tabs, not two-spaces.

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