Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Extract console helpers from Formatter #1377

Merged
merged 4 commits into from

2 participants

@JonRowe
Owner

I'd like to break down the formatters a little more, and this is the start of that, for which I'd like feedback on.

lib/rspec/core/formatters/console_codes.rb
((2 lines not shown))
+ module Core
+ module Formatters
+ module ConsoleCodes
+ VT100_CODES =
+ {
+ :black => 30,
+ :red => 31,
+ :green => 32,
+ :yellow => 33,
+ :blue => 34,
+ :magenta => 35,
+ :cyan => 36,
+ :white => 37,
+ :bold => 1,
+ }
+ VT100_CODE_VALUES = VT100_CODES.values
@myronmarston Owner

Any reason you dropped the to_set? Set#include? is O(1) whereas Array#include? is O(N).

@JonRowe Owner
JonRowe added a note

So we don't have to load the library, it seemed overkill to need it here, as the numbers are unique, I hadn't considered the lookup time though, hmm.

@myronmarston Owner

That's a valid point. Set uses a hash internally for constant time inclusion lookups, so let's use a hash.

@myronmarston Owner

In fact, it looks like Hash#invert will do what you want.

@JonRowe Owner
JonRowe added a note

:+1:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@myronmarston myronmarston commented on the diff
lib/rspec/core/formatters/console_codes.rb
((1 lines not shown))
+module RSpec
+ module Core
+ module Formatters
+ module ConsoleCodes
+ VT100_CODES =
+ {
+ :black => 30,
+ :red => 31,
+ :green => 32,
+ :yellow => 33,
+ :blue => 34,
+ :magenta => 35,
+ :cyan => 36,
+ :white => 37,
+ :bold => 1,
+ }
@myronmarston Owner

I prefer the original formatting, I think:

VT100_CODES = {
  :black => 30,
  ...
  :bold => 1
}

It's subjective, though, so not a merge blocker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/rspec/core/formatters/console_codes.rb
((26 lines not shown))
+ code_for(:white)
+ end
+ end
+ end
+
+ def wrap(text, code_or_symbol)
+ if enabled?
+ "\e[#{console_code_for(code_or_symbol)}m#{text}\e[0m"
+ else
+ text
+ end
+ end
+
+ def enabled?
+ RSpec.configuration.color_enabled?
+ end
@myronmarston Owner

Seems like console_code_for and enabled? should be private, since wrap is the only public method here, right?

Also, enabled? seems slightly like overkill given how simple it is and the fact that it's only used once. I'd probably eliminate it, but that's subjective and not a merge blocker.

@JonRowe Owner
JonRowe added a note

I want to expose console_code_for so that other formatters can make use of it (say they wanted to do both a foreground and background code for example)

@myronmarston Owner

If you want console_code_for to be public it should probably have some specs that call it directly.

Also, once you do that, should the color enabled check be moved into that method?

@JonRowe Owner
JonRowe added a note

Good call, found a bug that way too ;)

should the color enabled check be moved into that method?

what would the method return if colour was disabled though?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
spec/rspec/core/formatters/console_codes_spec.rb
@@ -0,0 +1,28 @@
+require 'spec_helper'
+require 'rspec/core/formatters/console_codes'
+
+RSpec.describe "RSpec::Core::Formatters::ConsoleCodes" do
+ let(:console_codes) { RSpec::Core::Formatters::ConsoleCodes }
+
+ before do
+ allow(RSpec.configuration).to receive(:color_enabled?) { true }
+ end
+
+ describe "#wrap" do
+ it "accepts a VT100 integer code and formats the text with it" do
+ expect(console_codes.wrap('abc', 32)).to eq "\e[32mabc\e[0m"
+ end
@myronmarston Owner

I think I'd prefer this formatting:

context "when given a VT100 integer code" do
  it "formats the text with it"
end

context "when given a symbolic color name" do
  it "translates it to the correct integer code and formats the text with it"
end

context "when given :bold" do
 it "formats the text as bold"
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
spec/rspec/core/formatters/console_codes_spec.rb
((7 lines not shown))
+ before do
+ allow(RSpec.configuration).to receive(:color_enabled?) { true }
+ end
+
+ describe "#wrap" do
+ it "accepts a VT100 integer code and formats the text with it" do
+ expect(console_codes.wrap('abc', 32)).to eq "\e[32mabc\e[0m"
+ end
+
+ it "accepts a symbol as a color parameter and translates it to the correct integer code, then formats the text with it" do
+ expect(console_codes.wrap('abc', :green)).to eq "\e[32mabc\e[0m"
+ end
+
+ it "accepts a non-default color symbol as a parameter and translates it to the correct integer code, then formats the text with it" do
+ expect(console_codes.wrap('abc', :cyan)).to eq "\e[36mabc\e[0m"
+ end
@myronmarston Owner

This and the last testcase don't look like they are exercising different code paths, and from the perspective of the ConsoleCodes module, :green isn't any more a default color symbol than :cyan is.

Do you think we need both?

@JonRowe Owner
JonRowe added a note

Oh yeah, I'm assuming it was added to check additional colour codes were implemented, and has no real value now.

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

Good change in general. Left some comments.

lib/rspec/core/formatters/console_codes.rb
((7 lines not shown))
+ :black => 30,
+ :red => 31,
+ :green => 32,
+ :yellow => 33,
+ :blue => 34,
+ :magenta => 35,
+ :cyan => 36,
+ :white => 37,
+ :bold => 1,
+ }
+ VT100_CODE_VALUES = VT100_CODES.invert
+
+ module_function
+
+ def console_code_for(code_or_symbol)
+ if VT100_CODE_VALUES[code_or_symbol]
@myronmarston Owner

Semantically, I think it would make more sense to use one of the predicates Hash provides for this purpose: key?, has_key? or member?.

@JonRowe Owner
JonRowe added a note

I'm more tempted to collapse this into a one liner...

@JonRowe Owner
JonRowe added a note

Tried it, changed my mind, I'll use your preference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
spec/rspec/core/formatters/console_codes_spec.rb
((3 lines not shown))
+
+RSpec.describe "RSpec::Core::Formatters::ConsoleCodes" do
+ let(:console_codes) { RSpec::Core::Formatters::ConsoleCodes }
+
+ describe "#console_code_for(code_or_symbol)" do
+ context "when given a VT100 integer code" do
+ it "returns the code" do
+ expect(console_codes.console_code_for(32)).to eq 32
+ end
+ end
+ context "when given a symbolic name" do
+ it "returns the code" do
+ expect(console_codes.console_code_for(:green)).to eq 32
+ end
+ end
+ context "when given a nonexistant code" do
@myronmarston Owner

Any reason you chose not to put blank lines between contexts here? I prefer that formatting and it's consistent with the formatting below.

@JonRowe Owner
JonRowe added a note

No, restored.

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

I think this is ready to go, note no changelog because this is mostly an internal refactor?

@myronmarston

Isn't ConsoleCodes meant to be a public API? If so, it needs a changelog entry. If not, then the deprecation notices you added shouldn't tell folks to use it.

The build is failing as well...that needs to be addressed before we merge.

@JonRowe JonRowe referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@JonRowe JonRowe merged commit 66f396f into master
@JonRowe JonRowe deleted the formatter_extract_console_helpers branch
@JonRowe
Owner

The build was only failing due a load order issue, which I fixed, and I've added both a changelog and docs so merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 11, 2014
  1. @JonRowe

    Extract colour support to helper

    JonRowe authored
    Colour code lookup is now implemented in a seperate module.
  2. @JonRowe
  3. @JonRowe

    Changelog for #1377

    JonRowe authored
    [skip ci]
  4. @JonRowe

    docs for console codes

    JonRowe authored
    [skip ci]
This page is out of date. Refresh to see the latest.
View
2  Changelog.md
@@ -9,6 +9,7 @@ Breaking Changes for 3.0.0:
* Refactor filter manager so that it no longer subclasses Hash and has a
tighter, more domain-specific interface. (Sergey Pchelincev)
* Remove legacy colours definitions from `BaseTextFormatter`. (Jon Rowe)
+* Remove console color definitions from `BaseTextFormatter`. (Jon Rowe)
Enhancements:
@@ -19,6 +20,7 @@ Enhancements:
* Migrate `execution_result` (exposed by metadata) from a hash to a
first-class object with appropriate attributes. It retains deprecated
hash behavior for backwards compatibility. (Myron Marston)
+* Provide console code helper for formatters. (Jon Rowe)
Bug Fixes:
View
33 lib/rspec/core/formatters/base_text_formatter.rb
@@ -1,5 +1,5 @@
RSpec::Support.require_rspec_core "formatters/base_formatter"
-require 'set'
+RSpec::Support.require_rspec_core "formatters/console_codes"
module RSpec
module Core
@@ -130,41 +130,14 @@ def close(notification)
output.close if IO === output && output != $stdout
end
- VT100_COLORS = {
- :black => 30,
- :red => 31,
- :green => 32,
- :yellow => 33,
- :blue => 34,
- :magenta => 35,
- :cyan => 36,
- :white => 37
- }
-
- VT100_COLOR_CODES = VT100_COLORS.values.to_set
-
- def color_code_for(code_or_symbol)
- if VT100_COLOR_CODES.include?(code_or_symbol)
- code_or_symbol
- else
- VT100_COLORS.fetch(code_or_symbol) do
- color_code_for(:white)
- end
- end
- end
-
- def colorize(text, code_or_symbol)
- "\e[#{color_code_for(code_or_symbol)}m#{text}\e[0m"
- end
-
protected
def bold(text)
- color_enabled? ? "\e[1m#{text}\e[0m" : text
+ ConsoleCodes.wrap(text, :bold)
end
def color(text, color_code)
- color_enabled? ? colorize(text, color_code) : text
+ ConsoleCodes.wrap(text, color_code)
end
def failure_color(text)
View
56 lib/rspec/core/formatters/console_codes.rb
@@ -0,0 +1,56 @@
+module RSpec
+ module Core
+ module Formatters
+ # ConsoleCodes provides helpers for formatting console output
+ # with ANSI codes, e.g. color's and bold.
+ module ConsoleCodes
+ VT100_CODES =
+ {
+ :black => 30,
+ :red => 31,
+ :green => 32,
+ :yellow => 33,
+ :blue => 34,
+ :magenta => 35,
+ :cyan => 36,
+ :white => 37,
+ :bold => 1,
+ }
@myronmarston Owner

I prefer the original formatting, I think:

VT100_CODES = {
  :black => 30,
  ...
  :bold => 1
}

It's subjective, though, so not a merge blocker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ VT100_CODE_VALUES = VT100_CODES.invert
+
+ module_function
+
+ # Fetches the correct code for the supplied symbol, or checks
+ # that a code is valid. Defaults to white (37).
+ #
+ # @param [Symbol, Fixnum] Symbol or code to check
+ # @return [Fixnum] a console code
+ def console_code_for(code_or_symbol)
+ if VT100_CODE_VALUES.has_key?(code_or_symbol)
+ code_or_symbol
+ else
+ VT100_CODES.fetch(code_or_symbol) do
+ console_code_for(:white)
+ end
+ end
+ end
+
+ # Wraps a peice of text in ANSI codes with the supplied code. Will
+ # only apply the control code if `RSpec.configuration.color_enabled?`
+ # returns true.
+ #
+ # @param text [String] the text to wrap
+ # @param code_or_symbol [Symbol, Fixnum] the desired control code
+ # @return [String] the wrapped text
+ def wrap(text, code_or_symbol)
+ if RSpec.configuration.color_enabled?
+ "\e[#{console_code_for(code_or_symbol)}m#{text}\e[0m"
+ else
+ text
+ end
+ end
+
+ end
+ end
+ end
+end
View
26 lib/rspec/core/formatters/legacy_formatter.rb
@@ -123,6 +123,31 @@ def white(text)
RSpec.deprecate("RSpec::Core::Formatters::BaseTextFormatter#white", :replacement => "#default_color")
color(text, :white)
end
+
+ module ConstantLookup
+ def const_missing(name)
+ base_name = "RSpec::Core::Formatters::BaseTextFormatter"
+ case name
+ when :VT100_COLORS then
+ RSpec.deprecate("#{base_name}::VT100_COLORS", :replacement => "RSpec::Core::Formatters::ConsoleCodes.code_for(code_or_symbol)")
+ RSpec::Core::Formatters::ConsoleCodes::VT100_CODES
+ when :VT100_COLOR_CODES then
+ RSpec.deprecate("#{base_name}::VT100_COLOR_CODES", :replacement => "RSpec::Core::Formatters::ConsoleCodes.code_for(code_or_symbol)")
+ RSpec::Core::Formatters::ConsoleCodes::VT100_CODE_VALUES
+ else
+ super
+ end
+ end
+ end
+
+ # These are part of the deprecated interface, so no individual deprecations
+ def color_code_for(code_or_symbol)
+ ConsoleCodes.console_code_for(code_or_symbol)
+ end
+
+ def colorize(text, code_or_symbol)
+ ConsoleCodes.wrap(text, code_or_symbol)
+ end
end
# @api private
@@ -140,6 +165,7 @@ def initialize(formatter_class, *args)
if defined?(BaseTextFormatter) && formatter_class.ancestors.include?(BaseTextFormatter)
formatter_class.class_eval do
include LegacyColorSupport
+ extend LegacyColorSupport::ConstantLookup
end
end
@formatter = formatter_class.new(*args)
View
13 spec/rspec/core/formatters/base_text_formatter_spec.rb
@@ -264,17 +264,4 @@ def run_all_and_dump_failures
end
end
- describe "#colorize" do
- it "accepts a VT100 integer code and formats the text with it" do
- expect(formatter.colorize('abc', 32)).to eq "\e[32mabc\e[0m"
- end
-
- it "accepts a symbol as a color parameter and translates it to the correct integer code, then formats the text with it" do
- expect(formatter.colorize('abc', :green)).to eq "\e[32mabc\e[0m"
- end
-
- it "accepts a non-default color symbol as a parameter and translates it to the correct integer code, then formats the text with it" do
- expect(formatter.colorize('abc', :cyan)).to eq "\e[36mabc\e[0m"
- end
- end
end
View
50 spec/rspec/core/formatters/console_codes_spec.rb
@@ -0,0 +1,50 @@
+require 'spec_helper'
+require 'rspec/core/formatters/console_codes'
+
+RSpec.describe "RSpec::Core::Formatters::ConsoleCodes" do
+ let(:console_codes) { RSpec::Core::Formatters::ConsoleCodes }
+
+ describe "#console_code_for(code_or_symbol)" do
+ context "when given a VT100 integer code" do
+ it "returns the code" do
+ expect(console_codes.console_code_for(32)).to eq 32
+ end
+ end
+
+ context "when given a symbolic name" do
+ it "returns the code" do
+ expect(console_codes.console_code_for(:green)).to eq 32
+ end
+ end
+
+ context "when given a nonexistant code" do
+ it "returns the code for white" do
+ expect(console_codes.console_code_for(:octarine)).to eq 37
+ end
+ end
+ end
+
+ describe "#wrap" do
+ before do
+ allow(RSpec.configuration).to receive(:color_enabled?) { true }
+ end
+
+ context "when given a VT100 integer code" do
+ it "formats the text with it" do
+ expect(console_codes.wrap('abc', 32)).to eq "\e[32mabc\e[0m"
+ end
+ end
+
+ context "when given a symbolic color name" do
+ it "translates it to the correct integer code and formats the text with it" do
+ expect(console_codes.wrap('abc', :green)).to eq "\e[32mabc\e[0m"
+ end
+ end
+
+ context "when given :bold" do
+ it "formats the text as bold" do
+ expect(console_codes.wrap('abc', :bold)).to eq "\e[1mabc\e[0m"
+ end
+ end
+ end
+end
View
22 spec/rspec/core/formatters/legacy_formatter_spec.rb
@@ -1,4 +1,5 @@
require 'spec_helper'
+require 'rspec/core/formatters/console_codes'
require 'rspec/core/formatters/legacy_formatter'
require 'support/old_style_formatter_example'
require 'support/legacy_formatter_using_sub_classing_example'
@@ -138,8 +139,25 @@ def initialize(*args)
describe LegacyFormatterUsingSubClassing do
let(:legacy_formatter) { formatter.formatter }
- described_class::VT100_COLORS.each do |name, number|
- next if name == :black
+ it "will lookup colour codes" do
+ expect(legacy_formatter.color_code_for(:black)).to eq 30
+ end
+
+ it "will colorize text" do
+ allow(RSpec.configuration).to receive(:color_enabled?) { true }
+ expect(legacy_formatter.colorize("text", :black)).to eq "\e[30mtext\e[0m"
+ end
+
+ it "allows access to the deprecated constants" do
+ legacy_formatter
+ expect_deprecation_with_call_site(__FILE__, __LINE__ + 1)
+ expect(described_class::VT100_COLORS).to eq ::RSpec::Core::Formatters::ConsoleCodes::VT100_CODES
+ expect_deprecation_with_call_site(__FILE__, __LINE__ + 1)
+ expect(described_class::VT100_COLOR_CODES).to eq ::RSpec::Core::Formatters::ConsoleCodes::VT100_CODE_VALUES
+ end
+
+ ::RSpec::Core::Formatters::ConsoleCodes::VT100_CODES.each do |name, number|
+ next if name == :black || name == :bold
describe "##{name}" do
before do
Something went wrong with that request. Please try again.