-
Notifications
You must be signed in to change notification settings - Fork 46
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
Implement Heroku style log colorization #57
Conversation
COLORS = [:cyan, :yellow, :green, :magenta, :red] | ||
|
||
def colorize(event) | ||
idx = (event.data["hostname"].length + event.data["program"].length) % 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great idea. I see one problem using lengths, though. If I have the hosts web1
, web2
, etc, the same program on all of them will be the same color.
What if we just summed up the unicode value of each character?
idx = (event.data["hostname"] + event.data["program"]).unpack('U*').inject(&:+) % 5
I don't know if this is the best way to get an integer value from a given string. It's just the first thing that came to mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a fantastic comment, and I was also wondering whether I'd like it. I encountered a similar problem where related hosts' output was the same color (your 1st example), but I actually preferred that output over many different colors (your 2nd example).
That said, I can imagine lots of cases where (a) the hostname+program lengths would be unintentionally overlapping, or (b) the overlap did indicate a relationship like webN
, but it wasn't the one I wanted.
What about combining these with another gap you identified, and changing --force-colors
into --colors={h,p,hp,none}
? The default would be --colors=hp
(your 2nd example) when color terminal emulation was detected, but someone could also use --colors=p
for the same behavior as Heroku's CLI (and your 1st example) or --colors=h
to make it easier to spot multiline output from the same sender.
One important thing to note is that we only have 5 characters, so color collisions are going to happen anyway. We can try to make them less of an issue, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, I'm all for the sum of the Unicode character values like you described.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about combining these with another gap you identified, and changing
--force-colors
into--colors={h,p,hp,none}
? The default would be--colors=hp
(your 2nd example) when color terminal emulation was detected, but someone could also use--colors=p
for the same behavior as Heroku's CLI (and your 1st example) or--colors=h
to make it easier to spot multiline output from the same sender.
I could see that being useful. I think that's worth exploring in a new PR. No sense holding up colors while we discuss if/how to accomplish it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about (event.data["hostname"] + event.data["program"]).hash % 5
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I knew there had to be a better way to turn a string into an integer.
Looks great. Can you add |
This PR implements #51. The output is so much easier to skim. Nice work @zakwilson. |
@@ -1,3 +1,5 @@ | |||
source 'https://rubygems.org' | |||
|
|||
gemspec | |||
|
|||
gem "ansi" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line can be removed as it's included as part of gemspec
Thanks, gents. Talked a bit more here and ended up with these changes. The bigger ones:
The smaller one:
For all of these, the last option should win. |
@zakwilson I just updated my comment from yesterday with 2 changes. |
Continuing in new PR from different branch (above). |
No description provided.