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

Add simplest TTY detection for Colorize #4075

Merged
merged 3 commits into from Apr 10, 2017

Conversation

makenowjust
Copy link
Contributor

Simplest version of #3925.

It renames @on to @enabled because Colorize::Object has #on method also, but @on is not related and its name does not make a sense.

It renames `@on` to `@enabled` because `Colorize::Object` has `#on`
method also, but `@on` is not related and its name does not make a sense.
@bcardiff bcardiff added this to the Next milestone Mar 17, 2017
Copy link
Member

@bcardiff bcardiff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@makenowjust would you mind adding a doc sentence regarding that by default colorize is enabled if both STDOUT and STDERR are tty or if Colorize.enabled= / #toggle is explicitly called?

Apologies for the delay on this. 🙏

If you won't be able let us know and I will pick it from here.

src/colorize.cr Outdated
@@ -158,11 +158,13 @@ struct Colorize::Object(T)
COLORS = %w(black red green yellow blue magenta cyan light_gray dark_gray light_red light_green light_yellow light_blue light_magenta light_cyan white)
MODES = %w(bold bright dim underline blink reverse hidden)

class_property? enabled : Bool = STDOUT.tty? && STDERR.tty?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this class_property should belong to module Colorize. I found it misleading to be inside the generic class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. It's my mistake. I fixed it and added doc comment to Colorize.enabled?.

@makenowjust
Copy link
Contributor Author

@asterite asterite modified the milestone: Next Apr 7, 2017
@bcardiff bcardiff merged commit cf207b0 into crystal-lang:master Apr 10, 2017
@asterite
Copy link
Member

Just a question: what if I want to send output to a file and later show it colorized in another way but still recognizing the color codes? For example like what travis does (stores colored output and probably formats it in the web with JS). I don't understand why the Colorize should be enabled or disabled according to one possible output (STDOUT and STDERR). I understand that this is the most probable output, but this will lead to a lot of confusion and unexpected behaviour.

I'd say to revert this merge, but I'd like to know others' opinions.

@asterite
Copy link
Member

For example:

# foo.cr
require "colorize"

# I want to write "hello" in red to a file
File.write("file.txt", "hello".colorize.red)

puts "Done!"

Now:

$ bin/crystal foo.cr
Done!
$ cat file.txt
hello # in red
$ bin/crystal foo.cr > another.txt
Done!
$ cat file.txt
hello # without colors

Totally unexpected behaviour.

Colorize shouldn't be tied to STDOUT/STDERR.

@bcardiff
Copy link
Member

This PR change the default setting from enabled to enabled if TTY.
And tidy up some names.

Colorize was already global and not sensible to the targeted IO. I agree it is an issue it should be addressed (for example in playground you can send the log with ansi colors despite the tty). But I see that as a separate issue.

@asterite
Copy link
Member

For example this is with Ruby's colorize:

require "colorize"

puts "hello".colorize(:red)

It outputs "hello" in red to the console. But if we redirect the output to a file with ruby foo.rb > file.txt and then cat file.txt we still get colors. And I don't think it's good to assume that just because we send the output to a file or some other device we don't want colors (for example using cat and having colors if very convenient).

@makenowjust What do you think about this?

@bew
Copy link
Contributor

bew commented Apr 10, 2017

I agree with @asterite 's last comment, colorize should not assume anything, but it should be easy for the user to disable the colors on demand.
We could have an helper like Colorize.on_tty_only! that checks stdout/err and disable colors when it's not a tty.

A use case would be to have a big colored output, and pipe it to less:

$ ./some_big_colored_output | less -R

The -R will interpret the colors and display them in the less pager..

@makenowjust
Copy link
Contributor Author

@asterite said:

(for example using cat and having colors if very convenient).

But I use cat for removing colors sometime (i.e. brew install foo | cat). Expected behavior is different between peoples. This makes harder colorize problem.

However I think I shouldn't have changed default behavior (backward compatibility is important and current behavior is difficult to understand). @bew's Colorize.on_tty_only! seems good idea. Thank you @bew.

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

Successfully merging this pull request may close these issues.

None yet

4 participants