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

Improve Colorize #7690

Closed
wants to merge 31 commits into from
Closed

Improve Colorize #7690

wants to merge 31 commits into from

Conversation

wooster0
Copy link
Contributor

@wooster0 wooster0 commented Apr 18, 2019

This:

  • Adds more overall documentation to Colorize and improves some.
  • Removes Colorize.on_tty_only! and makes it the default so you don't have to put it at the beginning of every program that uses Colorize because you don't want that your program behaves badly in non-TTY terminals (i.e. that the ANSI escape codes become visible). Most people don't do this/forget it.
  • Uses the explosive automatic casting feature instead of macros in order to support symbols which results in cleaner code and compile time errors instead of run time errors.
    Fixes Remove manual symbols in Crystal with automatic casting enums? #6736 because as to my knowledge there are no other examples where enums are being manually duplicated as symbols.
  • Adds Object.colorize(r, g, b) which is a syntactic sugar for Object.colorize(Colorize::ColorRGB.new(r, g, b)). It's a bit of an alias but I'd say it's rather syntactic sugar and less worse than all the other aliases in Colorize.
  • Fixes "hello".colorize(:red).inspect returning "hello" in red instead of "\e[31mhello\e[0m" which is probably what you want when you inspect a colored string.
  • Deprecates the alias bright. See Improve Colorize #7690 (comment).

src/colorize.cr Outdated Show resolved Hide resolved
src/colorize.cr Show resolved Hide resolved
src/colorize.cr Outdated Show resolved Hide resolved
src/colorize.cr Outdated Show resolved Hide resolved
src/colorize.cr Show resolved Hide resolved
src/colorize.cr Outdated Show resolved Hide resolved
wooster0 added 8 commits May 13, 2019 18:29
I would like to use these two methods in my code because I want to append only the start ANSI escape sequence, not the end. I want to append the end at a different time. This would allow me to implement something efficiently.
@wooster0
Copy link
Contributor Author

I resolved the conflicts, fixed some bugs and deprecated a method. Can this please be reviewed?

samples/2048.cr Show resolved Hide resolved
src/colorize.cr Outdated Show resolved Hide resolved
@Blacksmoke16
Copy link
Member

Bump, this still on anyone's radar? Seems it needs another rebase...

@wooster0
Copy link
Contributor Author

Hello, I don't think there is interest in this PR but I resolved the conflicts anyway. It's unfortunate that this wasn't merged before 1.0 because it does some breaking changes like removing on_tty_only. This makes it hard to merge now.

@straight-shoota
Copy link
Member

I think most of this is really good, but it's also a bunch of different changes merged together. It's hard to review. IMO it would be easier to merge in spearate chunks.

Ideally, I think each of the bullet points in the OP should be a separate PR. Maybe some could be merged, but to me it seems they're for the most part independent changes.

As you already mentioned, we can't simply remove on_tty_only! for backward compatibility. But there's no need to. We can just keep this method and make it deprecated.

Comment on lines +312 to +315
if mode == Mode::Bright
puts "Warning: The text decoration `bright` is deprecated. Please use `bold` instead.".colorize(:light_yellow) unless @@warning_printed
@@warning_printed = true
end
Copy link
Member

Choose a reason for hiding this comment

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

Do deprecation warnings work on enum members? Don't think the annotation existed when this PR was made but that would prob be more ideal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it would be nice if that worked but they don't work.

@wooster0
Copy link
Contributor Author

I have re-added on_tty_only! so this PR should no longer break anything.

@HertzDevil
Copy link
Contributor

Object.colorize(r, g, b) is resolved by #11832.

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.

Remove manual symbols in Crystal with automatic casting enums?
6 participants