Compiler: use Colorize.on_tty_only! #4499

makenowjust · 2017-06-03T03:18:51Z

Colorize.on_tty_only! is introduced in #4271, but it is not used in compiler. I think it is useful that colorize is turned on/off depending on tty detection result.

In addition, I fixed bin/crystal wrapper script to detect TTY. bin/crystal script's SUPPORTS_COLORS is really buggy, at least it isn't working correct now.

RX14 · 2017-06-03T08:20:12Z

Could you seperate the changes to bin/crystal and the compiler into seperate commits? The core team might even want them in seperate prs.

makenowjust · 2017-06-03T08:25:54Z

ok. Please wait.

makenowjust · 2017-06-03T10:00:19Z

@RX14 I can separate this PR into the wrapper bug fix and others (using Colorize.on_tty_support! in compiler). This bug fix is just one line, however it causes a conflict with this PR surely (because this PR added TTY detection into this line). Do you want it really?

RX14 · 2017-06-03T10:46:16Z

@makenowjust i'm not sure I understand what the conflict is.

makenowjust · 2017-06-03T10:53:44Z

@RX14 Bug fix patch:

diff --git a/bin/crystal b/bin/crystal
index 54704199d..8b4380f30 100755
--- a/bin/crystal
+++ b/bin/crystal
@@ -115,7 +115,7 @@ __has_colors() {
     return 1
   fi
 }
-SUPPORTS_COLORS=$(__has_colors)
+SUPPORTS_COLORS=$(__has_colors && echo true || echo false)
 __error_msg() {
   if $SUPPORTS_COLORS; then
     # bold red coloring

makenowjust · 2017-06-03T10:59:14Z

I didn't intend to fix a bug. I implemented TTY detection on the wrapper script correctly, then I fixed a bug unexpectedly.

Why cause a problem when I make a PR about Colorize!!!!

makenowjust · 2017-06-04T09:55:31Z

This PR diff is a little large, but it is really simple fix. I will explain:

Added Colorize.on_tty_only! to Crystal::Command#run.
Fixed --no-color option to Colorize.enabled = false, and added --color option as Colorize.enabled = true.
Removed @color instance variable and colorize, with_color method, then replaced colorize(obj) to obj.colorize. In addition, removed .toggle(@color) on commands/formatter.cr.
Two specs about compiler is broken, fixed it.
The wrapper is fixed to use colorize on TTY only.

That's all.

straight-shoota · 2017-11-28T19:57:49Z

@makenowjust could you rebase this PR on master?

To support TTY detection Fixed specs and wrapper script

makenowjust · 2017-11-30T03:33:27Z

@straight-shoota rebased.

straight-shoota · 2017-11-30T20:00:54Z

spec/compiler/semantic/generic_class_spec.cr

@@ -636,7 +636,9 @@ describe "Semantic: generic class" do
    begin
      semantic(nodes)
    rescue ex : TypeException
+      old_enabled, Colorize.enabled = Colorize.enabled?, false


This would be better as two statements.

Thank you but I don't think that.

straight-shoota · 2017-11-30T20:11:03Z

I'd suggest to add the option to explicitly set color mode to auto (which is the default). This would be additional to the already included --color and --no-color.
Example from man git-diff(1):

--color[=<when>]
Show colored diff.  --color (i.e. without =<when>) is the same as --color=always.  <when> can be one of always, never, or auto. [...]
 --no-color
Turn off colored diff. This can be used to override configuration settings. It is the same as --color=never.

A lot of command line application do it like this, and I think this would be the best way to handle this.

makenowjust · 2017-12-01T03:56:12Z

@straight-shoota It was implemented in #3925...

Sija · 2017-12-01T09:45:41Z

src/compiler/crystal/command.cr

+      case color
+      when "auto"
+        Colorize.on_tty_only!
+      when "always"


color = "always" if color.empty? => when "always", .empty?

when "always", ""

Personally, color = "always" if color.empty? shows "always" as default clearly, so I would keep this.

Maybe, but it adds an completely unnecessary comparison and assignment.

It should be all in one case statement, not a case plus an if.

makenowjust · 2017-12-01T14:38:15Z

bin/crystal run --color a.cr means bin/crystal run --color=a.cr, so --color's default value does not make a sense. I'll remove this.

For example `crystal run --color a.cr` means `crystal run --color=a.cr`. It is not useful.

straight-shoota · 2017-12-01T14:40:02Z

Why not use --color[=<always,never,auto>]?

makenowjust · 2017-12-01T14:41:24Z

@straight-shoota It does not change meaning.

straight-shoota · 2017-12-01T14:43:27Z

Yes it does. If --color expects a = to be assigned a value, --color a.cr will not be confused.

makenowjust · 2017-12-01T14:44:35Z

Yes but it is not implemented unfortunately.

straight-shoota · 2017-12-01T14:47:46Z

You mean OptionParser doesn't differentiate between --color=<when> and --color <when>? Then maybe it should or you could implement this differently.

The commands should provide an arg-less --color flag to enable colorization.

bew · 2017-12-01T14:50:47Z

Maybe it has already been said, but to me, the <when> is useless:

--color argument: force colors
--no-color argument: disable colors
no color-related argument: automatic colors (on tty)

straight-shoota · 2017-12-01T14:55:17Z

It is not useless but adds expressiveness.

It makes it easy to inject a configuration value into the command line arguments, like crystal --color=$MY_COLOR_SETTING ... instead of some complicated decision tree between --color, --no-color and ``.

larubujo · 2017-12-01T14:58:03Z

more complex things. global state in colorize used by all apps. global state is bad. the design is being ruined. more command line switches. the direction should be to simplify things.

straight-shoota · 2017-12-01T15:21:25Z

@larubujo This PR is about the compiler, not "all apps".

makenowjust · 2017-12-01T15:41:06Z

The commands should provide an arg-less --color flag to enable colorization.

I totally agree with this. But it can be merged this before implementing this feature in OptionParser, and then fixes this in PR about OptionParser.

RX14 · 2017-12-01T19:27:20Z

I personally prefer @bew's suggestion. At least, if this PR is blocked on an OptionParser enhancement, do that first.

makenowjust · 2017-12-01T19:38:06Z

@straight-shoota I want to revert --color=when support for now because it blocks to merge this PR and I don't think this option is needed strongly. If you want this feature really, then you can create a new pull request after fixed OptionParser.

makenowjust · 2017-12-01T19:39:14Z

Sorry... 🙇

jkthorne · 2018-05-26T07:08:52Z

@makenowjust are you still working on this?

makenowjust force-pushed the fix/colorize/use-in-crystal-command branch from e9f2a05 to a08ce00 Compare June 3, 2017 10:02

makenowjust mentioned this pull request Jul 27, 2017

Correctly work __has_colors check in bin/crystal #4751

Merged

makenowjust force-pushed the fix/colorize/use-in-crystal-command branch from a08ce00 to cfd74df Compare November 29, 2017 02:13

Compiler: use Colorize.on_tty_only!

d6c6e18

To support TTY detection Fixed specs and wrapper script

makenowjust force-pushed the fix/colorize/use-in-crystal-command branch from cfd74df to d6c6e18 Compare November 30, 2017 03:05

RX14 approved these changes Nov 30, 2017

View reviewed changes

straight-shoota reviewed Nov 30, 2017

View reviewed changes

Add --color=[when] option

1f2c2f5

Sija reviewed Dec 1, 2017

View reviewed changes

Remove --color default value

2364e68

For example `crystal run --color a.cr` means `crystal run --color=a.cr`. It is not useful.

straight-shoota mentioned this pull request Dec 1, 2017

OptionParser should allow flag with optional value #5338

Open

makenowjust closed this Dec 1, 2017

makenowjust reopened this Dec 1, 2017

ammarfaizi2 approved these changes Nov 30, 2018

View reviewed changes

straight-shoota added the pr:needs-work A PR requires modifications by the author. label Feb 11, 2019

wooster0 mentioned this pull request Aug 4, 2019

Refactor coloring in compiler #7252

Closed

HertzDevil mentioned this pull request Dec 3, 2021

GNU optional arguments in OptionParser #11537

Closed

HertzDevil mentioned this pull request Sep 8, 2022

CLI: --no-color should be the default if the output isn't a terminal #12461

Open

Blacksmoke16 added the topic:compiler label Sep 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compiler: use Colorize.on_tty_only! #4499

Compiler: use Colorize.on_tty_only! #4499

makenowjust commented Jun 3, 2017

RX14 commented Jun 3, 2017

makenowjust commented Jun 3, 2017

makenowjust commented Jun 3, 2017

RX14 commented Jun 3, 2017

makenowjust commented Jun 3, 2017

makenowjust commented Jun 3, 2017

makenowjust commented Jun 4, 2017

straight-shoota commented Nov 28, 2017

makenowjust commented Nov 30, 2017

straight-shoota Nov 30, 2017

makenowjust Dec 1, 2017

straight-shoota commented Nov 30, 2017 •

edited

makenowjust commented Dec 1, 2017

Sija Dec 1, 2017

makenowjust Dec 1, 2017

makenowjust Dec 1, 2017

straight-shoota Dec 1, 2017

straight-shoota Dec 1, 2017

makenowjust commented Dec 1, 2017

straight-shoota commented Dec 1, 2017 •

edited

makenowjust commented Dec 1, 2017

straight-shoota commented Dec 1, 2017

makenowjust commented Dec 1, 2017 •

edited

straight-shoota commented Dec 1, 2017

bew commented Dec 1, 2017

straight-shoota commented Dec 1, 2017

larubujo commented Dec 1, 2017

straight-shoota commented Dec 1, 2017

makenowjust commented Dec 1, 2017

RX14 commented Dec 1, 2017 •

edited

makenowjust commented Dec 1, 2017

makenowjust commented Dec 1, 2017

jkthorne commented May 26, 2018

Compiler: use Colorize.on_tty_only! #4499

Are you sure you want to change the base?

Compiler: use Colorize.on_tty_only! #4499

Conversation

makenowjust commented Jun 3, 2017

RX14 commented Jun 3, 2017

makenowjust commented Jun 3, 2017

makenowjust commented Jun 3, 2017

RX14 commented Jun 3, 2017

makenowjust commented Jun 3, 2017

makenowjust commented Jun 3, 2017

makenowjust commented Jun 4, 2017

straight-shoota commented Nov 28, 2017

makenowjust commented Nov 30, 2017

straight-shoota Nov 30, 2017

Choose a reason for hiding this comment

makenowjust Dec 1, 2017

Choose a reason for hiding this comment

straight-shoota commented Nov 30, 2017 • edited

makenowjust commented Dec 1, 2017

Sija Dec 1, 2017

Choose a reason for hiding this comment

makenowjust Dec 1, 2017

Choose a reason for hiding this comment

makenowjust Dec 1, 2017

Choose a reason for hiding this comment

straight-shoota Dec 1, 2017

Choose a reason for hiding this comment

straight-shoota Dec 1, 2017

Choose a reason for hiding this comment

makenowjust commented Dec 1, 2017

straight-shoota commented Dec 1, 2017 • edited

makenowjust commented Dec 1, 2017

straight-shoota commented Dec 1, 2017

makenowjust commented Dec 1, 2017 • edited

straight-shoota commented Dec 1, 2017

bew commented Dec 1, 2017

straight-shoota commented Dec 1, 2017

larubujo commented Dec 1, 2017

straight-shoota commented Dec 1, 2017

makenowjust commented Dec 1, 2017

RX14 commented Dec 1, 2017 • edited

makenowjust commented Dec 1, 2017

makenowjust commented Dec 1, 2017

jkthorne commented May 26, 2018

straight-shoota commented Nov 30, 2017 •

edited

straight-shoota commented Dec 1, 2017 •

edited

makenowjust commented Dec 1, 2017 •

edited

RX14 commented Dec 1, 2017 •

edited