From 00158ac407b214ac788e4a837771119692d92d45 Mon Sep 17 00:00:00 2001 From: Joe Stein Date: Fri, 20 Sep 2024 21:37:52 -0700 Subject: [PATCH 1/6] Remove unused Csi methods It's hard to keep Csi.color_enabled and SuperDiff::Configuration.color_enabled in sync. But Csi.color_enabled is only serving Csi.colorize, which in turn only exists for Csi.inspect_colors_in, which is apparently unused elsewhere in this project. Since Csi is a library private to this project, we can remove them. --- lib/super_diff.rb | 1 - lib/super_diff/core/configuration.rb | 6 ------ lib/super_diff/core/helpers.rb | 2 +- lib/super_diff/csi.rb | 32 ---------------------------- 4 files changed, 1 insertion(+), 40 deletions(-) diff --git a/lib/super_diff.rb b/lib/super_diff.rb index 349f9b52..e9e034cb 100644 --- a/lib/super_diff.rb +++ b/lib/super_diff.rb @@ -36,7 +36,6 @@ def self.const_missing(missing_const_name) def self.configure yield configuration - configuration.updated end def self.configuration diff --git a/lib/super_diff/core/configuration.rb b/lib/super_diff/core/configuration.rb index 0f5a533d..17087228 100644 --- a/lib/super_diff/core/configuration.rb +++ b/lib/super_diff/core/configuration.rb @@ -71,12 +71,6 @@ def merge!(configuration_or_options) end options.each { |key, value| instance_variable_set("@#{key}", value) } - - updated - end - - def updated - SuperDiff::Csi.color_enabled = color_enabled? end def add_extra_diff_formatter_classes(*classes) diff --git a/lib/super_diff/core/helpers.rb b/lib/super_diff/core/helpers.rb index 35dcc979..5b829b93 100644 --- a/lib/super_diff/core/helpers.rb +++ b/lib/super_diff/core/helpers.rb @@ -6,7 +6,7 @@ module Helpers # TODO: Simplify this def style(*args, color_enabled: true, **opts, &block) klass = - if color_enabled && Csi.color_enabled? + if color_enabled && SuperDiff.configuration.color_enabled? Csi::ColorizedDocument else Csi::UncolorizedDocument diff --git a/lib/super_diff/csi.rb b/lib/super_diff/csi.rb index 35c7327c..43956685 100644 --- a/lib/super_diff/csi.rb +++ b/lib/super_diff/csi.rb @@ -12,26 +12,10 @@ module Csi autoload :TwentyFourBitColor, "super_diff/csi/twenty_four_bit_color" autoload :UncolorizedDocument, "super_diff/csi/uncolorized_document" - class << self - attr_writer :color_enabled - end - def self.reset_sequence ResetSequence.new end - def self.color_enabled? - @color_enabled - end - - def self.colorize(*args, **opts, &block) - if color_enabled? - ColorizedDocument.new(*args, **opts, &block) - else - UncolorizedDocument.new(*args, **opts, &block) - end - end - def self.decolorize(text) text.gsub(/\e\[\d+(?:;\d+)*m(.+?)\e\[0m/, '\1') end @@ -39,21 +23,5 @@ def self.decolorize(text) def self.already_colorized?(text) text.match?(/\e\[\d+m/) end - - def self.inspect_colors_in(text) - [FourBitColor, EightBitColor, TwentyFourBitColor].reduce( - text - ) do |str, klass| - klass.sub_colorized_areas_in(str) do |area, color| - color_block = colorize("◼︎", color.to_foreground) - - layer_indicator = (color.foreground? ? "(fg)" : "(bg)") - - "#{color_block} #{layer_indicator} ❮#{area}❯" - end - end - end - - self.color_enabled = false end end From 79ef2668ae176497de573df0873ce989ac3c9853 Mon Sep 17 00:00:00 2001 From: Joe Stein Date: Fri, 20 Sep 2024 21:45:48 -0700 Subject: [PATCH 2/6] Default to RSpec colorization config when applicable When `color_enabled` has not been explicitly set and `super_diff/rspec` has been loaded (i.e. whoever is using this gem has indicated they want to load the RSpec extensions, not just that RSpec is available), use the RSpec color mode if one hasn't been explicitly set already. --- lib/super_diff/core/configuration.rb | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/lib/super_diff/core/configuration.rb b/lib/super_diff/core/configuration.rb index 17087228..596e52f4 100644 --- a/lib/super_diff/core/configuration.rb +++ b/lib/super_diff/core/configuration.rb @@ -23,7 +23,7 @@ class Configuration def initialize(options = {}) @actual_color = :yellow @border_color = :blue - @color_enabled = color_enabled_by_default? + @color_enabled = nil @diff_elision_enabled = false @diff_elision_maximum = 0 @elision_marker_color = :cyan @@ -41,6 +41,8 @@ def initialize(options = {}) def initialize_dup(original) super + @extra_diff_formatter_classes = + original.extra_diff_formatter_classes.dup.freeze @extra_differ_classes = original.extra_differ_classes.dup.freeze @extra_operation_tree_builder_classes = original.extra_operation_tree_builder_classes.dup.freeze @@ -51,6 +53,8 @@ def initialize_dup(original) end def color_enabled? + return color_enabled_by_default? if @color_enabled.nil? + @color_enabled end @@ -159,7 +163,7 @@ def to_h { actual_color: actual_color, border_color: border_color, - color_enabled: color_enabled?, + color_enabled: @color_enabled, diff_elision_enabled: diff_elision_enabled?, diff_elision_maximum: diff_elision_maximum, elision_marker_color: elision_marker_color, @@ -179,7 +183,13 @@ def to_h private def color_enabled_by_default? - ENV["CI"] == "true" || $stdout.respond_to?(:tty?) && $stdout.tty? + return true if ENV["CI"] == "true" + + if defined?(::SuperDiff::RSpec) + return ::RSpec.configuration.color_enabled? + end + + $stdout.respond_to?(:tty?) && $stdout.tty? end end end From 0c19a964b87924202649ee59afab71fdbbbeb682 Mon Sep 17 00:00:00 2001 From: Joe Stein Date: Fri, 20 Sep 2024 22:07:41 -0700 Subject: [PATCH 3/6] Add tests for SuperDiff::Core::Configuration --- spec/spec_helper.rb | 1 + spec/unit/core/configuration_no_rspec_spec.rb | 58 ++++++ spec/unit/core/configuration_spec.rb | 176 ++++++++++++++++++ 3 files changed, 235 insertions(+) create mode 100644 spec/unit/core/configuration_no_rspec_spec.rb create mode 100644 spec/unit/core/configuration_spec.rb diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 751e3fa4..5bb4569b 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -68,6 +68,7 @@ unless defined?(ActiveSupport) config.filter_run_excluding active_support: true end + config.filter_run_excluding with_superdiff_rspec: false config.order = :random Kernel.srand config.seed diff --git a/spec/unit/core/configuration_no_rspec_spec.rb b/spec/unit/core/configuration_no_rspec_spec.rb new file mode 100644 index 00000000..82241840 --- /dev/null +++ b/spec/unit/core/configuration_no_rspec_spec.rb @@ -0,0 +1,58 @@ +require "delegate" +require "super_diff" + +class FakeTTYDecorator < SimpleDelegator + def initialize(obj, is_tty:) + super(obj) + @is_tty = is_tty + end + + def isatty = @is_tty + def tty? = isatty +end + +RSpec.describe SuperDiff::Core::Configuration, with_superdiff_rspec: false do + describe "#color_enabled?" do + it "is true when stdout is a TTY" do + original_stdout = $stdout + color_enabled = nil + begin + $stdout = FakeTTYDecorator.new(StringIO.new, is_tty: true) + color_enabled = SuperDiff.configuration.color_enabled? + ensure + $stdout = original_stdout + end + expect(color_enabled).to be(true) + end + + it "is false when stdout is not a TTY but we are in CI" do + original_stdout = $stdout + original_ci = ENV["CI"] + color_enabled = nil + begin + $stdout = FakeTTYDecorator.new(StringIO.new, is_tty: false) + ENV["CI"] = "true" + color_enabled = SuperDiff.configuration.color_enabled? + ensure + $stdout = original_stdout + ENV["CI"] = original_ci + end + expect(color_enabled).to be(true) + end + + it "is false when stdout is not a TTY and we are not in CI" do + original_stdout = $stdout + original_ci = ENV["CI"] + color_enabled = nil + begin + $stdout = FakeTTYDecorator.new(StringIO.new, is_tty: false) + ENV["CI"] = nil + color_enabled = SuperDiff.configuration.color_enabled? + ensure + $stdout = original_stdout + ENV["CI"] = original_ci + end + expect(color_enabled).to be(false) + end + end +end diff --git a/spec/unit/core/configuration_spec.rb b/spec/unit/core/configuration_spec.rb new file mode 100644 index 00000000..e479754e --- /dev/null +++ b/spec/unit/core/configuration_spec.rb @@ -0,0 +1,176 @@ +require "spec_helper" + +RSpec.describe SuperDiff::Core::Configuration do + it "maintains frozen instance variables" do + expect(described_class.new.instance_variables).to all(be_frozen) + end + + describe ".new" do + context "when passed nothing" do + subject(:config) { described_class.new } + + it "creates a Configuration object with reasonable defaults" do + expect(config.actual_color).to eq(:yellow) + end + end + + context "when passed options" do + subject(:config) { described_class.new(actual_color: :cyan) } + + it "overrides the defaults with the provided options" do + expect(config.actual_color).to eq(:cyan) + end + + it "uses the defaults for other options" do + expect(config.border_color).to eq(:blue) + end + end + end + + describe ".dup" do + subject(:duplicated_config) { original_config.dup } + + let(:original_config) { described_class.new(overrides) } + let(:in_both) { Class.new } + let(:in_duplicated_only) { Class.new } + let(:in_original_only) { Class.new } + + let(:overrides) do + { + extra_diff_formatter_classes: [], + extra_differ_classes: [], + extra_inspection_tree_builder_classes: [], + extra_operation_tree_builder_classes: [], + extra_operation_tree_classes: [] + } + end + + %i[ + diff_formatter + differ + operation_tree_builder + operation_tree + inspection_tree_builder + ].each do |object_type| + it "duplicates extra #{object_type.to_s.tr("_", " ")} classes" do + add_method_name = :"add_extra_#{object_type}_class" + get_method_name = :"extra_#{object_type}_classes" + + original_config.send(add_method_name, in_both) + expect { + duplicated_config.send(get_method_name) << in_duplicated_only + }.to raise_error(FrozenError) + duplicated_config.send(add_method_name, in_duplicated_only) + original_config.send(add_method_name, in_original_only) + + expect(original_config.send(get_method_name)).to include( + in_both, + in_original_only + ) + expect(original_config.send(get_method_name)).not_to include( + in_duplicated_only + ) + + expect(duplicated_config.send(get_method_name)).to include( + in_both, + in_duplicated_only + ) + expect(duplicated_config.send(get_method_name)).not_to include( + in_original_only + ) + end + end + end + + %i[ + diff_formatter + differ + operation_tree_builder + operation_tree + inspection_tree_builder + ].each do |object_type| + describe "#add_extra_#{object_type}_classes" do + let(:config) { described_class.new } + let(:new_class1) { Class.new } + let(:new_class2) { Class.new } + + it "appends multiple given classes" do + config.send("add_extra_#{object_type}_classes", new_class1, new_class2) + expect(config.send("extra_#{object_type}_classes")[-2..]).to eq( + [new_class1, new_class2] + ) + end + + it "appends a single given class" do + config.send("add_extra_#{object_type}_classes", new_class1) + expect(config.send("extra_#{object_type}_classes")[-1]).to eq( + new_class1 + ) + end + end + + describe "#prepend_extra_#{object_type}_classes" do + let(:config) { described_class.new } + let(:new_class1) { Class.new } + let(:new_class2) { Class.new } + + it "prepends multiple given classes" do + config.send( + "prepend_extra_#{object_type}_classes", + new_class1, + new_class2 + ) + expect(config.send("extra_#{object_type}_classes")[..1]).to eq( + [new_class1, new_class2] + ) + end + + it "prepends a single given class" do + config.send("prepend_extra_#{object_type}_classes", new_class1) + expect(config.send("extra_#{object_type}_classes")[0]).to eq(new_class1) + end + end + end + + describe "#color_enabled?" do + context "when explicitly set" do + it "equals what was set" do + [true, false].each do |value| + SuperDiff.configuration.color_enabled.tap do |original| + SuperDiff.configuration.color_enabled = value + expect(SuperDiff.configuration.color_enabled?).to be(value) + SuperDiff.configuration.color_enabled = original + end + end + end + end + + context "when not explicitly set" do + context 'when ENV["CI"] is true' do + it "is true" do + color_enabled = nil + ClimateControl.modify(CI: "true") do + color_enabled = SuperDiff.configuration.color_enabled? + end + expect(color_enabled).to be(true) + end + end + + context 'when ENV["CI"] is not true' do + it "defaults to RSpec's config" do + ClimateControl.modify(CI: nil) do + %i[automatic on off].each do |value| + RSpec.configuration.color_mode.tap do |original| + RSpec.configuration.color_mode = value + expect(SuperDiff.configuration.color_enabled?).to eq( + RSpec.configuration.color_enabled? + ) + RSpec.configuration.color_mode = original + end + end + end + end + end + end + end +end From 8663e5e7af0c1c5959ff4ef2ce2b524ee51a1055 Mon Sep 17 00:00:00 2001 From: Joe Stein Date: Fri, 20 Sep 2024 22:28:31 -0700 Subject: [PATCH 4/6] Add changes to changelog --- CHANGELOG.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 186f3133..737f2cae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,13 @@ ### Features -- Add better support for Data object diffing. [#259](https://github.com/splitwise/super_diff/pull/224) +- Add better support for Data object diffing. [#259](https://github.com/splitwise/super_diff/pull/259) +- Fall back on RSpec color mode when `SuperDiff.configuration.color_enabled` is unspecified or nil. [#261](https://github.com/splitwise/super_diff/pull/261) + +### Breaking changes + +- Removed several `SuperDiff::Csi` methods. This will break any code that uses those parts of the `SuperDiff::Csi` (which is private in general). +- `SuperDiff.configuration.color_enabled = nil` used to disable color output. It now allows SuperDiff to determine whether to colorize output based on the environment (namely RSpec color mode and whether stdout is a TTY). ## 0.12.1 - 2024-04-26 From e5f93d4edad4fe00762f823d6bbc4838a9d83ad6 Mon Sep 17 00:00:00 2001 From: Joe Stein Date: Fri, 20 Sep 2024 23:42:13 -0700 Subject: [PATCH 5/6] Update docs --- docs/users/customization.md | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/docs/users/customization.md b/docs/users/customization.md index ebb42f75..13e233b4 100644 --- a/docs/users/customization.md +++ b/docs/users/customization.md @@ -14,17 +14,17 @@ end The following is a list of options you can set on the configuration object along with their defaults: -| name | description | default | -| ---------------------- | ----------------------------------------------------------------------------- | ----------------------------------------------------------- | -| `actual_color` | The color used to display "actual" values in diffs | `:yellow` | -| `border_color` | The color used to display the border in diff keys | `:blue` | -| `color_enabled` | Whether to colorize output | `true` if `ENV["CI"]` or stdout is a TTY, `false` otherwise | -| `diff_elision_enabled` | Whether to elide (remove) unchanged lines in diff | `false` | -| `diff_elision_maximum` | How large a section of consecutive unchanged lines can be before being elided | `0` | -| `elision_marker_color` | The color used to display the marker substituted for elided lines in a diff | `:cyan` | -| `expected_color` | The color used to display "expected" values in diffs | `:magenta` | -| `header_color` | The color used to display the "Diff:" header in failure messages | `:white` | -| `key_enabled` | Whether to show the key above diffs | `true` | +| name | description | default | +| ---------------------- | ------------------------------------------------------------------------------------- | ---------- | +| `actual_color` | The color used to display "actual" values in diffs | `:yellow` | +| `border_color` | The color used to display the border in diff keys | `:blue` | +| `color_enabled` | Whether to colorize output, or `nil` to let SuperDiff decide based on the environment | `nil` | +| `diff_elision_enabled` | Whether to elide (remove) unchanged lines in diff | `false` | +| `diff_elision_maximum` | How large a section of consecutive unchanged lines can be before being elided | `0` | +| `elision_marker_color` | The color used to display the marker substituted for elided lines in a diff | `:cyan` | +| `expected_color` | The color used to display "expected" values in diffs | `:magenta` | +| `header_color` | The color used to display the "Diff:" header in failure messages | `:white` | +| `key_enabled` | Whether to show the key above diffs | `true` | The following is a list of methods you can call on the configuration object: From e5ba527f9a6c1779be1ebb40b5e9b32af7a09e6f Mon Sep 17 00:00:00 2001 From: Joe Stein Date: Fri, 20 Sep 2024 23:56:20 -0700 Subject: [PATCH 6/6] Fix docsite workflow --- .github/workflows/super_diff.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/super_diff.yml b/.github/workflows/super_diff.yml index f1672e7e..a9771a39 100644 --- a/.github/workflows/super_diff.yml +++ b/.github/workflows/super_diff.yml @@ -143,7 +143,7 @@ jobs: env: IS_NEW_RELEASE: ${{ needs.collect-release-info.outputs.IS_NEW_RELEASE }} RELEASE_VERSION: ${{ needs.collect-release-info.outputs.RELEASE_VERSION }} - BRANCH_NAME: ${{ github.ref_name }} + BRANCH_NAME: ${{ github.head_ref || github.ref_name }} COMMIT_ID: ${{ github.event.pull_request.head.sha }} outputs: DOCSITE_RELEASE_VERSION: ${{ steps.command.outputs.DOCSITE_RELEASE_VERSION }}