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

Refactor coloring in compiler #7252

Open
wants to merge 14 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@r00ster91
Copy link
Contributor

r00ster91 commented Jan 2, 2019

This:

  • Resolves unnecessary colorizing complexity in the compiler.
    Originally I just wanted to fix that when you do '', that the did-you-mean in the error is colored in bold yellow (currently it's not) but it turned out to be actually quite difficult to retrieve the @color value from the Compiler (whether --no-color has been passed or not) from this spot in the lexer.
    So now at the beginning of a compilation, Colorize.enabled is simply used to turn colors off when --no-color has been passed. Which is much simpler than having to define a colorize method first and then having to figure out how to even retrieve the color value from the compiler. Now strings can be colored wherever you want just like you would in your own code without colorize helper methods.
  • Removes 2 unnecessary "colorize" requires.
  • Fixes some did-you-means or other helpful hints not being colored in bold yellow in multiple places.
  • For example Time.new(seconds: 0) results in the error
    error
    with the last item not being colored in bold/white (or more specifically with the items after the (trying this one) not being colored in bold/white). That's fixed as well.
Show resolved Hide resolved src/compiler/crystal/util.cr Outdated

@r00ster91 r00ster91 force-pushed the r00ster91:color branch 2 times, most recently from c6c178c to 06ca67f Jan 2, 2019

@r00ster91 r00ster91 force-pushed the r00ster91:color branch from 06ca67f to c36c722 Jan 3, 2019

r00ster91 added some commits Jan 3, 2019

@r00ster91 r00ster91 force-pushed the r00ster91:color branch from ec40c94 to 7a1c3c4 Jan 3, 2019

r00ster91 added some commits Jan 3, 2019

@r00ster91

This comment has been minimized.

Copy link
Contributor

r00ster91 commented Jan 3, 2019

Phew CI finally passes! That took way too long.

@@ -394,10 +394,12 @@ class Crystal::Call
str << "\n - "
append_def_full_name a_def.owner, a_def, arg_types, str
if defs.size > 1 && a_def.same?(matched_def)
str << colorize(" (trying this one)").blue
str << " (trying this one)".colorize.blue
str << "\e[1m" # Make the rest of the string bold

This comment has been minimized.

@Sija

Sija Jan 3, 2019

Contributor

This seems hackish... isn't there any other way to avoid using raw ansi escape codes?

ditto below

This comment has been minimized.

@r00ster91

r00ster91 Jan 4, 2019

Contributor

Yes, I know. I would really like to avoid this but I couldn't find a way so far.
Let's cut the problem down to this:

require "colorize"

str = IO::Memory.new

str << "\n - "
str << "item 1"
str << "\n - "
str << "item 2"

str << " (did you mean this one?)".colorize.yellow

str << "\n - "
str << "item 3"

puts str.to_s.colorize.bold

did you mean

Internally the string is:

"\e[1m\n - item 1\n - item 2\e[33m (did you mean this one?)\e[0m\n - item 3\e[0m"
 ^                          ^                              ^               ^
 Bold                       Yellow                         Terminates      Terminates
                                                           Yellow          Bold

The problem here is that the Terminates Yellow escape sequence doesn't just terminate Yellow but also Bold while actually the last sequence (Terminates Bold) is supposed to terminate Bold.
And that's fixed by appending a new Bold ("\e[1m") after Terminates Yellow:

did you mean

This comment has been minimized.

@straight-shoota

straight-shoota Jan 4, 2019

Member

You just need to call colorize.bold on each individual component instead of the entire list.

This comment has been minimized.

@straight-shoota

straight-shoota Jan 4, 2019

Member

Might seem a bit irrational as it could normally be summarized. But that's the way these colorize commands work. You can't nest them.

This comment has been minimized.

@r00ster91

r00ster91 Jan 10, 2019

Contributor

I figured out now why the specs are failing.
Currently I'm colorizing each item invidually bold but the whole error message is also getting colorized bold at the end nearly before it's getting raised. Thus the error ends in \e[0m\e[0m. So I could fix the specs by manually appending \e[0m at the end. Or by colorizing everything here in this string build invidually too: call_error.cr:553 and then disabling that the whole string gets colorized bold by adding an ugly parameter here: exception.cr:140 just for this one case. But I don't think that's the right way.
But I can't really think of anything else which does not involve using ANSI escape sequences. Maybe modifying Colorize but I also don't think that's the right way because the user just expects \e[0m\e[0m to be at the end in this case, not \e[0m.
I don't think having some ANSI escape sequences in the specs or the code is that bad when there's also a comment.

This comment has been minimized.

@j8r

j8r Jan 10, 2019

Contributor

@r00ster91 if you want to reset the colors, you can use Colorize.reset instead of directly the raw ANSI escape code.

This comment has been minimized.

@r00ster91

r00ster91 Jan 11, 2019

Contributor

Colorize.reset only works for IO's, not strings.

r00ster91 added some commits Jan 5, 2019

Show resolved Hide resolved spec/compiler/semantic/generic_class_spec.cr Outdated
Show resolved Hide resolved src/compiler/crystal/util.cr
Show resolved Hide resolved src/compiler/crystal/util.cr Outdated

r00ster91 added some commits Jan 9, 2019

Show resolved Hide resolved src/colorize.cr
Show resolved Hide resolved src/colorize.cr

r00ster91 added some commits Jan 14, 2019

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