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

add Label to LogMessage and colorize it #95

Merged
merged 2 commits into from
Apr 11, 2017
Merged

add Label to LogMessage and colorize it #95

merged 2 commits into from
Apr 11, 2017

Conversation

klyonrad
Copy link

This adds the level of the LogMessage as uppercase label and colorizes it when the level is WARN and above.

Inspired by the :pretty formatter

Closes #94

@klyonrad
Copy link
Author

@mattbrictson Hey, could you please help me out with that Test Suite? I can only get the tests passing for older SSHKit versions if I remove the color codes from the to asserted output.

@mattbrictson
Copy link
Owner

Thanks for the PR! I am a bit busy and may not have time to review this right away. If there are any Airbrussh contributors/reviewers lurking on this thread, feel free to jump in. 😄

@ghost
Copy link

ghost commented Mar 15, 2017

would love to see this merged!

@klyonrad is there any way to try your solution locally?

@klyonrad
Copy link
Author

Ouf. Been a while since I coded that.

@daniel-gomes-sociomantic you can always apply the modified code in your project 😉

Regarding the Pull Request, this requirement has to be satisfied:

I'm willing to accept a PR that adds a colorized WARN, ERROR or FATAL prefix for those levels (leaving the log message itself un-colorized). No prefix or color for INFO.

which (judging from my own code comments) complied to

@mattbrictson
Copy link
Owner

@daniel-gomes-sociomantic if you'd like to try this code in your project, specify airbrussh like this in your project's Gemfile:

gem "airbrussh", :github => "klyonrad/airbrussh", :branch => "colorize-logmessage"

Then make sure to bundle install and run capistrano via:

bundle exec cap

@ncreuschling
Copy link

I would like to see this incorporated.

return print_indented_line(log_message.to_s) if verbosity < 2
# use level string to call corresponding colorize method
level = send(LEVEL_COLORS[verbosity], LEVEL_NAMES[verbosity].ljust(6))
print_indented_line(level + log_message.to_s)
Copy link
Owner

Choose a reason for hiding this comment

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

The behavior looks good, but I think this code is a bit fragile because it depends on the verbosity levels being certain integer values. For example if verbosity < 2 is not very clear when it could be written as if verbosity < SSHKit::Logger::WARN.

What do you think about using a case statement instead? Like this:

def write_log_message(log_message)
  return if debug?(log_message)
  print_task_if_changed
  print_indented_line(format_log_message(log_message))
end

def format_log_message(log_message)
  case log_message.verbosity
  when SSHKit::Logger::WARN
    "WARN  #{yellow(log_message.to_s)}"
  when SSHKit::Logger::ERROR
    "ERROR #{red(log_message.to_s)}"
  when SSHKit::Logger::FATAL
    "FATAL #{red(log_message.to_s)}"
  else
    log_message.to_s
  end
end

Copy link
Author

@klyonrad klyonrad Apr 10, 2017

Choose a reason for hiding this comment

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

well it's more clear but needs more code lines 😉

gonna do it with that

however you reversed the implementation of your own wish

leaving the log message itself un-colorized

@mattbrictson
Copy link
Owner

Hey, could you please help me out with that Test Suite? I can only get the tests passing for older SSHKit versions if I remove the color codes from the to asserted output.

Actually the tests look good. If you are running Ruby 2.4, you'll get unrelated failures for older SSHKit versions because they are incompatible with 2.4. The Travis build is passing, which is what matters.

If you could rebase on master and address the implementation comment above, I will be happy to merge. Thanks!

@klyonrad
Copy link
Author

Actually the tests look good. If you are running Ruby 2.4, you'll get unrelated failures for older SSHKit versions because they are incompatible with 2.4. The Travis build is passing, which is what matters.

Well, yeah the test worked because I removed checking the color 😉
I reverted that, so you can look into the travis output, see https://travis-ci.org/mattbrictson/airbrussh/builds/220481968

But that revert commit can also be deleted again

@mattbrictson
Copy link
Owner

Ah, sorry I missed that. Do you mind if I add a commit to your branch as an experiment to see if I can get the tests working?

@klyonrad
Copy link
Author

Do as you please! :)

@mattbrictson
Copy link
Owner

OK, take a look at 4163da1 and let me know what you think!

The `red`, `yellow`, etc. helpers within `formatter_test.rb` are
specifically used only for testing logfile output, not console output.
Since color is not present in logfile output for SSHKit > 1.7.1, these
helpers essentially do nothing in the latest SSHKit.

Therefore, when asserting color console output, it is necessary to use
the actual ANSI color control codes, as this commit does in the
`test_log_message_levels`. Note that tests that require color must
explicitly turn on color using `airbrussh_config.color = true`.

I removed the color expectations from `test_handles_rake_tasks`, since
color is not enabled for that test.
@klyonrad
Copy link
Author

looks good, I deleted two middle commits and then force-pushed; so now it's one commit from and one from you :)

@mattbrictson
Copy link
Owner

Great, I'll merge it in. Thanks for the PR and for your patience!

@mattbrictson mattbrictson merged commit 3b912fb into mattbrictson:master Apr 11, 2017
jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Apr 22, 2017
## [1.2.0][] (2017-04-14)

* [#95](mattbrictson/airbrussh#95): colorize LogMessage label on WARN level and above - [@klyonrad](https://github.com/klyonrad)
* [#106](mattbrictson/airbrussh#106): Remove the `log_file` parameter from the `CommandFormatter#exit_message` method; it was unused - [@mattbrictson](https://github.com/mattbrictson)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants