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

Remove the need for an external dependency for ANSI color #263

Merged
merged 4 commits into from
Jun 25, 2015

Conversation

mattbrictson
Copy link
Member

I think SSHKit's ANSI color needs are simple enough that we do not need to depend on a third-party gem (colorize). The colorize dependency has been a source of bugs.

Instead, we could implement a simple module that provides various colorizing methods. These methods could be used as module functions, or mixed in to the class where they are needed for convenience.

I did this for airbrussh, and it has worked well so far:

module Airbrussh
  # Very basic support for ANSI color, so that we don't have to rely on
  # any external dependencies.
  module Colors
    ANSI_CODES = {
      :red    => 31,
      :green  => 32,
      :yellow => 33,
      :blue   => 34,
      :gray   => 90
    }.freeze

    module_function

    # Define red, green, blue, etc. methods that return a copy of the
    # String that is wrapped in the corresponding ANSI color escape
    # sequence.
    ANSI_CODES.each do |name, code|
      define_method(name) do |string|
        "\e[0;#{code};49m#{string}\e[0m"
      end
    end
  end
end

If there is interest I can create a PR for SSHKit.

@robd
Copy link
Contributor

robd commented Jun 24, 2015

I think this would be good. The main (only?) place in SSHKit where this is used is the Color class. I am in favour of having an API with a single method name (eg colorize) where the color is passed in as a symbol (rather than separate methods for each color). This makes the code easier if you need to pass the colour around without having to call send(:color_name), and it also means that IDEs don't get confused when searching for usages.

The Pretty formatter also uses bold formatting, so the API could be simplified in that respect because it currently accepts any of the colorize formats (:underline, :blink etc), but only :bold is needed.

@leehambley
Copy link
Member

If been holding off on this because I rarely run into the colorize bug, and
printing colours without respecting terminfo is bad practice, but let's
take a step in the right direction here!
On 24 Jun 2015 9:40 pm, "Rob Dupuis" [email protected] wrote:

I think this would be good. The main (only?) place in SSHKit where this is
used is the Color
https://github.com/capistrano/sshkit/blob/master/lib/sshkit/color.rb
class. I am in favour of having an API with a single method name (eg
colorize) where the color is passed in as a symbol (rather than separate
methods for each color). This makes the code easier if you need to pass
the colour around

color = case stream_type

without having to call send(:color_name), and it also means that IDEs
don't get confused when searching for usages.

The Pretty formatter also uses bold formatting, so the API could be
simplified in that respect because it currently accepts any of the colorize
formats (:underline, :blink etc
https://github.com/fazibear/colorize/blob/0523bea5747f9b7b823c79b2935bdd880cb301e8/lib/colorize/class_methods.rb#L77-L82
).


Reply to this email directly or view it on GitHub
#263 (comment).

@mattbrictson
Copy link
Member Author

Great! I'll get a PR started.

@mattbrictson
Copy link
Member Author

Voilà! Transformed this issue into a PR. Let me know what you think.

@leehambley
Copy link
Member

Merged :) Thanks @mattbrictson.

Terminal colour code support, and font weighting, etc is a much more complex topic than many realise, and I hope to make time to do a decent integration in SSH kit sometime that actually works correctly.

leehambley added a commit that referenced this pull request Jun 25, 2015
Remove the need for an external dependency for ANSI color
@leehambley leehambley merged commit c840d4f into capistrano:master Jun 25, 2015
jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Dec 14, 2015
## 1.8.1

  * Change license to MIT, thanks to all the patient contributors who gave
    their permissions.

## 1.8.0

  * add SSHKit::Backend::ConnectionPool#close_connections
    [PR #285](capistrano/sshkit#285)
    @akm
  * Clean up rubocop lint warnings
    [PR #275](capistrano/sshkit#275)
    @cshaffer
    * Prepend unused parameter names with an underscore
    * Prefer “safe assignment in condition”
    * Disambiguate regexp literals with parens
    * Prefer `sprintf` over `String#%`
    * No longer shadow `caller_line` variable in `DeprecationLogger`
    * Rescue `StandardError` instead of `Exception`
    * Remove useless `private` access modifier in `TestAbstract`
    * Disambiguate block operator with parens
    * Disambiguate between grouped expression and method params
    * Remove assertion in `TestHost#test_assert_hosts_compare_equal` that compares something with itself
  * Export environment variables and execute command in a subshell.
    [PR #273](capistrano/sshkit#273)
    @kuon
  * Introduce `log_command_start`, `log_command_data`, `log_command_exit` methods on `Formatter`
    [PR #257](capistrano/sshkit#257)
    @robd
    * Deprecate `@stdout` and `@stderr` accessors on `Command`
  * Add support for deprecation logging options.
    [README](README.md#deprecation-warnings),
    [PR #258](capistrano/sshkit#258)
    @robd
  * Quote environment variable values.
    [PR #250](capistrano/sshkit#250)
    @Sinjo - Chris Sinjakli
  * Simplified formatter hierarchy.
    [PR #248](capistrano/sshkit#248)
    @robd
    * `SimpleText` formatter now extends `Pretty`, rather than duplicating.
  * Hide ANSI color escape sequences when outputting to a file.
    [README](README.md#output-colors),
    [Issue #245](capistrano/sshkit#245),
    [PR #246](capistrano/sshkit#246)
    @robd
    * Now only color the output if it is associated with a tty,
      or the `SSHKIT_COLOR` environment variable is set.
  * Removed broken support for assigning an `IO` to the `output` config option.
    [Issue #243](capistrano/sshkit#243),
    [PR #244](capistrano/sshkit#244)
    @robd
    * Use `SSHKit.config.output = SSHKit::Formatter::SimpleText.new($stdin)` instead
  * Added support for `:interaction_handler` option on commands.
    [PR #234](capistrano/sshkit#234),
    [PR #242](capistrano/sshkit#242)
    @robd
  * Removed partially supported `TRACE` log level.
    [2aa7890](capistrano/sshkit@2aa7890)
    @robd
  * Add support for the `:strip` option to the `capture` method and strip by default on the `Local` backend.
    [PR #239](capistrano/sshkit#239),
    [PR #249](capistrano/sshkit#249)
    @robd
    * The `Local` backend now strips by default to be consistent with the `Netssh` one.
    * This reverses change [7d15a9a](capistrano/sshkit@7d15a9a) to the `Local` capture API to remove stripping by default.
    * If you require the raw, unstripped output, pass the `strip: false` option: `capture(:ls, strip: false)`
  * Simplified backend hierarchy.
    [PR #235](capistrano/sshkit#235),
    [PR #237](capistrano/sshkit#237)
    @robd
    * Moved duplicate implementations of `make`, `rake`, `test`, `capture`, `background` on to `Abstract` backend.
    * Backend implementations now only need to implement `execute_command`, `upload!` and `download!`
    * Removed `Printer` from backend hierarchy for `Local` and `Netssh` backends (they now just extend `Abstract`)
    * Removed unused `Net::SSH:LogLevelShim`
  * Removed dependency on the `colorize` gem. SSHKit now implements its own ANSI color logic, with no external dependencies. Note that SSHKit now only supports the `:bold` or plain modes. Other modes will be gracefully ignored. [#263](capistrano/sshkit#263)
  * New API for setting the formatter: `use_format`. This differs from `format=` in that it accepts options or arguments that will be passed to the formatter's constructor. The `format=` syntax will be deprecated in a future release. [#295](capistrano/sshkit#295)
  * SSHKit now immediately raises a `NameError` if you try to set a formatter that does not exist. [#295](capistrano/sshkit#295)
jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Dec 14, 2015
## 1.8.1

  * Change license to MIT, thanks to all the patient contributors who gave
    their permissions.

## 1.8.0

  * add SSHKit::Backend::ConnectionPool#close_connections
    [PR #285](capistrano/sshkit#285)
    @akm
  * Clean up rubocop lint warnings
    [PR #275](capistrano/sshkit#275)
    @cshaffer
    * Prepend unused parameter names with an underscore
    * Prefer “safe assignment in condition”
    * Disambiguate regexp literals with parens
    * Prefer `sprintf` over `String#%`
    * No longer shadow `caller_line` variable in `DeprecationLogger`
    * Rescue `StandardError` instead of `Exception`
    * Remove useless `private` access modifier in `TestAbstract`
    * Disambiguate block operator with parens
    * Disambiguate between grouped expression and method params
    * Remove assertion in `TestHost#test_assert_hosts_compare_equal` that compares something with itself
  * Export environment variables and execute command in a subshell.
    [PR #273](capistrano/sshkit#273)
    @kuon
  * Introduce `log_command_start`, `log_command_data`, `log_command_exit` methods on `Formatter`
    [PR #257](capistrano/sshkit#257)
    @robd
    * Deprecate `@stdout` and `@stderr` accessors on `Command`
  * Add support for deprecation logging options.
    [README](README.md#deprecation-warnings),
    [PR #258](capistrano/sshkit#258)
    @robd
  * Quote environment variable values.
    [PR #250](capistrano/sshkit#250)
    @Sinjo - Chris Sinjakli
  * Simplified formatter hierarchy.
    [PR #248](capistrano/sshkit#248)
    @robd
    * `SimpleText` formatter now extends `Pretty`, rather than duplicating.
  * Hide ANSI color escape sequences when outputting to a file.
    [README](README.md#output-colors),
    [Issue #245](capistrano/sshkit#245),
    [PR #246](capistrano/sshkit#246)
    @robd
    * Now only color the output if it is associated with a tty,
      or the `SSHKIT_COLOR` environment variable is set.
  * Removed broken support for assigning an `IO` to the `output` config option.
    [Issue #243](capistrano/sshkit#243),
    [PR #244](capistrano/sshkit#244)
    @robd
    * Use `SSHKit.config.output = SSHKit::Formatter::SimpleText.new($stdin)` instead
  * Added support for `:interaction_handler` option on commands.
    [PR #234](capistrano/sshkit#234),
    [PR #242](capistrano/sshkit#242)
    @robd
  * Removed partially supported `TRACE` log level.
    [2aa7890](capistrano/sshkit@2aa7890)
    @robd
  * Add support for the `:strip` option to the `capture` method and strip by default on the `Local` backend.
    [PR #239](capistrano/sshkit#239),
    [PR #249](capistrano/sshkit#249)
    @robd
    * The `Local` backend now strips by default to be consistent with the `Netssh` one.
    * This reverses change [7d15a9a](capistrano/sshkit@7d15a9a) to the `Local` capture API to remove stripping by default.
    * If you require the raw, unstripped output, pass the `strip: false` option: `capture(:ls, strip: false)`
  * Simplified backend hierarchy.
    [PR #235](capistrano/sshkit#235),
    [PR #237](capistrano/sshkit#237)
    @robd
    * Moved duplicate implementations of `make`, `rake`, `test`, `capture`, `background` on to `Abstract` backend.
    * Backend implementations now only need to implement `execute_command`, `upload!` and `download!`
    * Removed `Printer` from backend hierarchy for `Local` and `Netssh` backends (they now just extend `Abstract`)
    * Removed unused `Net::SSH:LogLevelShim`
  * Removed dependency on the `colorize` gem. SSHKit now implements its own ANSI color logic, with no external dependencies. Note that SSHKit now only supports the `:bold` or plain modes. Other modes will be gracefully ignored. [#263](capistrano/sshkit#263)
  * New API for setting the formatter: `use_format`. This differs from `format=` in that it accepts options or arguments that will be passed to the formatter's constructor. The `format=` syntax will be deprecated in a future release. [#295](capistrano/sshkit#295)
  * SSHKit now immediately raises a `NameError` if you try to set a formatter that does not exist. [#295](capistrano/sshkit#295)
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