[WIP] Added colour to post-compile errors #5325

hanyuone · 2017-11-25T00:32:06Z

Extra fixes to #4968.

A few problems with the PR so far:

callstack_spec.cr doesn't pass right now, for obvious reasons. I'll edit the spec files once this is finalised.
The colours in callstack.cr might not be to everyone's liking - I personally am fine with this, and the colours are more like placeholders to highlight the important parts of the error.
I'm personally not sure if the colour constants in the namespace are good practice.
I changed around the error formatting in exception.cr to have it like IndexError: Index out of bounds. I'm not sure if this is good or not.

Sija · 2017-11-25T06:02:34Z

src/callstack.cr

@@ -31,6 +31,16 @@ struct CallStack
    dir
  end

+  # ANSI color and formatting escape codes


Why not use Colorize for that?

Wait, there's a builtin library for this in Crystal? Huh, did not know, will fix ASAP.

Also @Sija, do you feel that the current colour scheme for errors is appropriate?

A screenshot would help

There is Colorize but it should not be used for this. Including it here would make it a mandatory dependency for each Crystal program. This should be avoided and as the current code shows, it's certainly easy enough to do without using a specialized component.

@RX14 Here is the error log with this program (I placed it in tmp/temp.cr):

def foo bar end def bar a = [1] a[2] end foo

straight-shoota · 2017-11-25T16:52:19Z

Colorized output should not be mandatory for every use case. For example, you might not want ANSI command sequences in stderr that is piped to a logfile.

There should at least be a option to disable this, but it would be nice if it would include some tty detection (like STDERR.tty?). I'm not sure if this is sufficient, there are probably a few other angles to cover.

Implementing colored output is easy, but figuring out when to apply is the real challenge. It's certainly worth looking at solutions used in related projects.

hanyuone · 2017-11-25T23:39:03Z

@straight-shoota Definitely. I'll have a look at Crystal's pre-compile errors to see what's happening there. I'll move the ANSI flags in exception.cr into a formatter function, or something like that.

I'm not sure if any existing languages have post-compile flags that are colorised with detail (i.e. more detail than Python's errors, which are just completely red). I'll try to find some examples.

Also, should colorized output be opt-in or opt-out?

hanyuone · 2017-11-26T01:13:28Z

Should I include post-compile errors into the --no-color flag, or should it be a separate flag?

straight-shoota · 2017-11-26T12:19:07Z

src/callstack.cr

@@ -152,6 +162,14 @@ struct CallStack
    end
  end

+  private def colorize_address(address)


This method could be more generalized and used in all places in both files. It should probably be an (undocumented) class method on Exception.

def Exception.colorize(code, string) if colorize? "#{code}#{string}\e[0m" else string end end

Added this method into Exception with the new commit!

straight-shoota · 2017-11-26T12:21:48Z

I don't think a compiler flag is the right thing for this because colour mode needs to be determined/configurable at runtime.

As a quick sketch, I'd suggest something similar like:

By default, error output is colorized if printed to a tty. I think it is reasonable to make this opt-out.
This default behaviour can be disabled at runtime (e.g. Exception.colorize = false). This allows applications to configure if error output should be colorized, maybe by using a user-setting like --no-color.

ysbaddaden · 2017-11-27T13:44:57Z

It's definitely nice, but I believe mixing bold / non bold is more disturbing than helpful. I'd stick to non-bold colors maybe.

ysbaddaden · 2017-11-27T13:46:46Z

Also this pushes Colorize from the stdlib (required on demand) to the corelib (always required).

straight-shoota · 2017-11-27T14:23:35Z

@ysbaddaden It currently does not use Colorize and there is no need to. There should not be any changes in this regard.

luislavena · 2017-11-28T15:05:28Z

Hello @Qwerp-Derp, this is a neat idea and nice start for a PR!

As commented above by other contributors, perhaps will be great to review the implementation to take the following in consideration:

Add a mechanism to disable exception colorization (ie. Exception.colorize = false)
Do not introduce a new compile-time definition (raw_err) to keep things simple
Acknowledge if STDERR is a TTY and only colorize in that scenario, which will avoid ANSI pollution when piping to another process or redirecting to a log file.

I think it will be also possible to add specs for first and last points, which will be a great addition.

Looking forward your updates and once again, thank you for your contribution and interest in Crystal!
❤️ ❤️ ❤️

Fryguy · 2017-11-28T23:08:31Z

src/exception.cr

+    if message.nil?
+      "\e[1m\e[31m" + self.class.name + "\e[0m"
+    else
+      "\e[1m\e[31m" + self.class.name + ": \e[0m\e[1m" + message.not_nil! + "\e[0m"


Instead of building up strings with +, this method should probably take the io object as a parameter and use <<

Fixed with new commit!

Fryguy · 2017-11-28T23:08:46Z

src/exception.cr

  def inspect_with_backtrace(io : IO)
-    io << message << " (" << self.class << ")\n"
+    trace = {% if flag?(:raw_err) %}
+      self.class.name + (message.nil? ? "" : ": #{message}")


Same here, you should probably use IO#<<

hanyuone · 2017-11-29T01:00:34Z

@straight-shoota You mean --no-color on the end of crystal file.cr, right? This name's already taken by controlling colours for pre-compile errors. Should I just add post-compile error colourisation into this flag, or should I introduce a new flag?

makenowjust · 2017-11-29T02:03:25Z

I have a plan to separate colorize into Colorize core module and other method extending (Object#colorize) because I will implement colorized PrettyPrinter and Colorize core module can be shared by them.

straight-shoota · 2017-11-29T12:49:53Z

@Qwerp-Derp No, I mean, that an application can implement a --no-color flag at runtime (that's not your job). For this to work, it needs to be able to configure the behaviour of error colorization (that's your job).

It might also make sense to override the default behaviour to always include ANSI codes, even when not writing to a tty. So the configuration needs the options auto (default), on and off.

larubujo · 2017-11-29T16:26:18Z

are you guys really considering adding colored output to all runtime exceptions? i've never seen that in any other language. it surprises me that you are even considering that

you want colored output? do it in your code. this doesn't belong in the standard library

Fryguy · 2017-11-29T16:28:29Z

@larubujo Elm does it, and their error messages are considered some of the best in the industry.

Fryguy · 2017-11-29T16:31:34Z

@Qwerp-Derp Do you have screenshots on both black backgrounds and non-black backgrounds?

larubujo · 2017-11-29T16:40:53Z

@Fryguy Elm does it? are you talking about compile-time errors or runtime errors? can you show an example?

Fryguy · 2017-11-29T16:49:39Z

@larubujo http://elm-lang.org/blog/compiler-errors-for-humans#colors-and-formatting . For an example, just make a syntax error in one of the online examples (e.g. http://elm-lang.org/examples/hello-html) and hit Compile.

larubujo · 2017-11-29T16:51:54Z

@Fryguy thats my point. thats a compile-time error. and crystal shows colors in compile-time errors (for example did you mean and other things). but runtime errors are a totally different thing. no language shows colors in there. what if you put that in a file. thats plain wrong. runtime errors in elm probably happen in the browser, so no colors there.

straight-shoota · 2017-11-29T16:53:42Z

@larubujo What would speak against the idea except (almost) "nobody else does it"?

As I have mentioned before, it will need some sane defaults checking if it makes sense to colorize. Writing to a file is no tty, so it shouldn't be colored.

Fryguy · 2017-11-29T16:54:39Z

Ah thanks for the clarification @larubujo... I didn't realize you were making a distinction between compile-time versus runtime and that one should have it but not the other...I thought you meant neither should have it. It's an interesting point.

larubujo · 2017-11-29T17:00:06Z

@straight-shoota what benefit do colors add for runtime errors? what if i dont like the colors? and you are talking about configuring this in all apps. increasing complexity. definitely not something for the standard lib. maybe no other language does it because its just plain wrong. and useless.

RX14 · 2017-11-29T17:32:53Z

Can you explain why you think runtime errors are different from compile-time ones regarding colourization?

faustinoaq · 2017-12-05T01:07:16Z

For me colorized runtime errors look a bit weird, maybe we can make it optional, depending on programmer preference:

require "colorize"
require "option_parser"

OptionParser.parse! do |parser|
  parser.banner = "Usage: salute [arguments]"
  parser.on("-c", "--color", "Colorize error output") do
    Colorize.errors = true
  end
end

Then:

$ saluto --color

Papierkorb · 2017-12-06T10:53:37Z

@faustinoaq If at all, that property should live in Exception (or a related class). I don't want Colorize, which until now is a random monkey-patching piece of code in the stdlib to control how my errors are output.

Besides the configuration checking if it's outputting to a TTY and the (theoretical) Exception.colorize_output=, there should also be a ENV["CRYSTAL_COLORIZED_EXCEPTIONS"]? == "1" of sorts. This would allow system administrators to choose a global default for their programs.

In this scenario, the chain would be:

If the ENV variable is set, initialize the class property to what it says
If no ENV var exists, default to if output is a TTy or not
Allow manual configuration through the class property with a stern warning in the docs that writing to this property can break behaviour expected (because configured) user behaviour.

ysbaddaden · 2017-12-06T12:18:34Z

Sigh. How to turn a simple feature to help development into a configuration hell. Let's get back to the core idea: help the developer. Nothing more.

If the output is a TTY and it's not a release build then enable colored backtraces. Period. There are no need for further configuration, especially at runtime, especially for production builds.

RX14 · 2017-12-06T14:56:45Z

I vote to forget it. Adding colours to the stacktrace adds very little and there will always be people who dislike it and want to turn it off.

hanyuone · 2017-12-06T23:20:40Z

@RX14, so is this request getting cancelled?

Also, sorry everyone for being inactive recently - I was busy with personal things.

RX14 · 2017-12-06T23:23:25Z

@Qwerp-Derp I don't make decisions for crystal, we as a team make decisions. I can't say if we'll close this issue or not. I vote against it, if the rest of the core team wants it it'll probably get in.

But yes, I think it's much more trouble than it's worth.

faustinoaq · 2017-12-06T23:27:10Z

@Qwerp-Derp I don't think Colorize Exceptions must be default on core crystal but maybe a shard, WDYT? Would be possible to convert this PR into a shard? 😅

hanyuone · 2017-12-06T23:37:09Z

How do I check to see if the --release flag has been passed to command.cr?

hugoabonizio · 2017-12-07T11:12:45Z

@Qwerp-Derp use {% if flag?(:release) %}, see

crystal/src/benchmark.cr

Lines 89 to 91 in 007899a

    
           {% if !flag?(:release) %} 
        
             puts "Warning: benchmarking without the `--release` flag won't yield useful results" 
        
           {% end %}

hanyuone · 2017-12-07T22:30:29Z

@hugoabonizio Thanks for the help! I included it in my recent commit.

hanyuone · 2017-12-07T22:31:51Z

@faustinoaq I personally don't think this would work as a shard, since this requires a close connection to the compiler and error messages itself. If the Crystal team doesn't approve of this, I might consider changing this into a shard, but it would require more effort since I don't have direct access to the compiler.

Sija · 2017-12-08T00:41:55Z

src/exception.cr

@@ -21,6 +21,11 @@ class Exception
  property callstack : CallStack?

  def initialize(@message : String? = nil, @cause : Exception? = nil)
+    {% if flag?(:release) %}


Ooops, this will get re-initialized on every Exception.new.

This might be moar up your alley:

class_getter colorize : Bool = {% if flag?(:release) %} STDERR.tty? {% else %} false {% end %}

Ah, that's neat. Will change now!

hanyuone · 2017-12-08T07:39:10Z

@RX14 Just curious, who's in the Crystal team? I know asterite and bcardiff are in it (I don't think I should tag them just yet). Anyone else?

hugoabonizio · 2017-12-08T11:01:55Z

@Qwerp-Derp https://github.com/orgs/crystal-lang/people

straight-shoota · 2017-12-08T11:27:25Z

@Qwerp-Derp This feature does not touch the compiler, all your edits are in the standard library. Through reopening and monkey-patching this could easily be implemented as a shard, but these practices should obviously be avoided if possible.

I think this would be an excellent feature for the standard library and I think it can be implemented in a way that it is non-intrusive but still configurable without incinerating configuration hell.

hanyuone · 2017-12-15T23:27:54Z

Bump - any updates to this thread?

hanyuone · 2017-12-31T11:31:23Z

Another bump - @RX14, @asterite, any updates on this issue?

RX14 · 2017-12-31T16:41:17Z

I like the idea of using only bold, no colours.

hanyuone · 2018-01-01T00:45:35Z

@RX14 The problem I can see with that is bold text doesn't add that much to distinguish between it and normal text - the whole point of the PR is to improve readability by a large amount, otherwise there wouldn't be much need for the PR at all. Adding colours would allow core parts of the error to be distinguished at a glance.

straight-shoota · 2018-01-01T10:51:38Z

I don't really get it why you would prefer "only bold" over coloured output. Colour has a some clear benefits. It doesn't just emphasise text (which can work better or less depending on the font face) but also conveys meaning, a colour encodes a certain message (red = error, danger), It is possible to highlight different aspects with different colours. Of course, it shouldn't be painted in vivid colours, but a few accents would be a great improvement.

It is true that some people have difficulties with colour perception. Tis usually doesn't mean to see no color at all but to have problems differentiating colours. In the worst case, this would not be worse than the status quo. For everyone else, colours are clearly an improvement because it structures and highlights the output.
It is very common for terminal applications to colourize output for improved readability, for example git or grep, syntax highlighting in the editor, etc. Even the Crystal compiler already uses coloured output for commands like spec, init and format.

asterite · 2018-01-02T00:31:33Z

For this code:

def foo
  [1][2]
end

foo

I get:

Index out of bounds (IndexError)
  from /usr/local/Cellar/crystal-lang/0.24.1_1/src/indexable.cr:0:17 in 'at'
  from /usr/local/Cellar/crystal-lang/0.24.1_1/src/indexable.cr:73:5 in '[]'
  from foo.cr:2:3 in 'foo'
  from foo.cr:5:1 in '__crystal_main'
  from /usr/local/Cellar/crystal-lang/0.24.1_1/src/crystal/main.cr:11:3 in '_crystal_main'
  from /usr/local/Cellar/crystal-lang/0.24.1_1/src/crystal/main.cr:112:5 in 'main_user_code'
  from /usr/local/Cellar/crystal-lang/0.24.1_1/src/crystal/main.cr:101:7 in 'main'
  from /usr/local/Cellar/crystal-lang/0.24.1_1/src/crystal/main.cr:135:3 in 'main'

First line is the error message and exception.
Subsequent lines are locations and method names.

There's just that. I don't think adding colors, or even bold text, improves the way you are going to perceive or handle that error.

The compiler uses bold text for some stuff, but it could simply just not do that. The few cases where color is used is to highlight pieces of code with squiggly marks, and yellow text for suggestions (did you mean?). Here it's just two pieces of information.

So, this added complexity is not worth it.

Plus, this can be really easily implemented as a shard: just redefine Exception#inspect_with_backtrace (with a bit of peeking into Exceptions's source code), so you can have this feature on demand.

RX14 · 2018-01-02T00:53:04Z

@asterite I do really wish that the symbol names were first and the file names were second though, or there was some kind of alignment. The symbol names are nowhere near each other in terms of line spacing, because the file names are so variable in length.

asterite · 2018-01-02T02:19:45Z

It's easy: we put them on a separate line with an extra indentation... Either the name or the location. I think both Go and Python do this (can't remember now).

hanyuone · 2019-08-15T12:50:45Z

Bump (since I saw this PR still existed). I might as well close this PR for now and reopen this as an issue, since Crystal has changed a lot in two(!) years and I'll probably need to do a complete rewrite. Plus, the proposal itself is pretty split, from what I've gathered in the comments.

Qwerp-Derp added 3 commits November 24, 2017 23:18

Colorized errors

2d2f525

Added colour to error messages

0835e57

Remove unnecessary ANSI flags

9467755

Sija reviewed Nov 25, 2017

View reviewed changes

Qwerp-Derp added 2 commits November 26, 2017 12:09

Added command-line option for no colour

17ed24d

Changed error message to bold instead of red

5a99928

straight-shoota reviewed Nov 26, 2017

View reviewed changes

Fryguy reviewed Nov 28, 2017

View reviewed changes

Added support for release flags

5f650cb

hanyuone changed the title ~~Added colour to post-compile errors~~ [WIP] Added colour to post-compile errors Dec 7, 2017

Sija reviewed Dec 8, 2017

View reviewed changes

Qwerp-Derp added 2 commits December 8, 2017 20:03

Fixed up callstack.cr to sync with exception.cr

1f292bf

Patched callstack and exception again

4331f8c

hanyuone closed this Aug 15, 2019

hanyuone mentioned this pull request Aug 15, 2019

Should colour be added to runtime errors (similar to compilation errors)? #8084

Closed

[WIP] Added colour to post-compile errors #5325

[WIP] Added colour to post-compile errors #5325

Conversation

hanyuone commented Nov 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straight-shoota commented Nov 25, 2017 • edited Loading

hanyuone commented Nov 25, 2017 • edited Loading

hanyuone commented Nov 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straight-shoota commented Nov 26, 2017

ysbaddaden commented Nov 27, 2017

ysbaddaden commented Nov 27, 2017

straight-shoota commented Nov 27, 2017

luislavena commented Nov 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanyuone commented Nov 29, 2017 • edited Loading

makenowjust commented Nov 29, 2017

straight-shoota commented Nov 29, 2017

larubujo commented Nov 29, 2017

Fryguy commented Nov 29, 2017

Fryguy commented Nov 29, 2017

larubujo commented Nov 29, 2017

Fryguy commented Nov 29, 2017

larubujo commented Nov 29, 2017

straight-shoota commented Nov 29, 2017 • edited Loading

Fryguy commented Nov 29, 2017

larubujo commented Nov 29, 2017 • edited Loading

RX14 commented Nov 29, 2017

faustinoaq commented Dec 5, 2017

Papierkorb commented Dec 6, 2017

ysbaddaden commented Dec 6, 2017

RX14 commented Dec 6, 2017 • edited Loading

hanyuone commented Dec 6, 2017

RX14 commented Dec 6, 2017 • edited Loading

faustinoaq commented Dec 6, 2017

hanyuone commented Dec 6, 2017

hugoabonizio commented Dec 7, 2017

hanyuone commented Dec 7, 2017

hanyuone commented Dec 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanyuone commented Dec 8, 2017

hugoabonizio commented Dec 8, 2017

straight-shoota commented Dec 8, 2017

hanyuone commented Dec 15, 2017

hanyuone commented Dec 31, 2017

RX14 commented Dec 31, 2017

hanyuone commented Jan 1, 2018

straight-shoota commented Jan 1, 2018

asterite commented Jan 2, 2018

RX14 commented Jan 2, 2018

asterite commented Jan 2, 2018

hanyuone commented Aug 15, 2019

hanyuone commented Nov 25, 2017 •

edited

Loading

straight-shoota commented Nov 25, 2017 •

edited

Loading

hanyuone commented Nov 25, 2017 •

edited

Loading

hanyuone commented Nov 29, 2017 •

edited

Loading

straight-shoota commented Nov 29, 2017 •

edited

Loading

larubujo commented Nov 29, 2017 •

edited

Loading

RX14 commented Dec 6, 2017 •

edited

Loading

RX14 commented Dec 6, 2017 •

edited

Loading