refactor: derived config logic moves to SnapDiff::Config (ADR-008 step 7b)

lib/capybara/screenshot/diff/config_legacy.rb

@@ -3,11 +3,13 @@
 # Legacy Capybara::Screenshot / Capybara::Screenshot::Diff config surface.
 #
 # Since ADR-008 step 1 the storage lives in SnapDiff::Config -- the require
-# leaf of the config graph (see its own header) -- and this file installs
-# the old accessor names as thin delegators onto SnapDiff.config, generated
-# from SnapDiff::Config::MAPPING. The v1 write surface
-# (Capybara::Screenshot.window_size = ..., Diff.configure { ... }) keeps
-# working unchanged: one storage, two views.
+# leaf of the config graph (see its own header) -- and since step 7b the
+# DERIVED values (active?, screenshot_area, default_options) live there
+# too. snap_diff/config.rb also generates the old accessor names as thin
+# delegators from SnapDiff::Config::MAPPING, so nothing but forwarders is
+# left here. The v1 surface (Capybara::Screenshot.window_size = ...,
+# Diff.configure { ... }, Diff.compare) keeps working unchanged: one
+# storage, two views.
 #
 # Load order: requiring snap_diff/config first also eagerly evaluates the
 # require-time defaults (ENV["CI"] for fail_if_new, Rails.root/pwd for
@@ -23,18 +25,15 @@ module Capybara
   module Screenshot
     class << self
       def active?
-        enabled || (enabled.nil? && Diff.enabled)
+        SnapDiff.config.active?
       end
 
       def screenshot_area
-        parts = [Screenshot.save_path]
-        parts << SnapDiff::Os.name if Screenshot.add_os_path
-        parts << Capybara.current_driver.to_s if Screenshot.add_driver_path
-        File.join(*parts)
+        SnapDiff.config.screenshot_area
       end
 
       def screenshot_area_abs
-        root / screenshot_area
+        SnapDiff.config.screenshot_area_abs
       end
     end
 
@@ -50,46 +49,18 @@ module Diff
       #     diff.driver = :vips
       #     diff.tolerance = 0.0005
       #   end
+      # The bare `yield` (rather than an explicit &block) keeps this
+      # method's published arity byte-identical to what it always had.
       def self.configure
-        yield Screenshot, self
+        SnapDiff.start { |screenshot, diff| yield screenshot, diff }
       end
 
       def self.compare(baseline_path, current_path, **options)
-        SnapDiff::Comparison.new(current_path, baseline_path, default_options.merge(options))
+        SnapDiff.compare(baseline_path, current_path, **options)
       end
 
       def self.default_options
-        {
-          area_size_limit: area_size_limit,
-          color_distance_limit: color_distance_limit,
-          driver: driver,
-          screenshot_format: Screenshot.screenshot_format,
-          capybara_screenshot_options: Screenshot.capybara_screenshot_options,
-          perceptual_threshold: perceptual_threshold,
-          shift_distance_limit: shift_distance_limit,
-          skip_area: skip_area,
-          stability_time_limit: Screenshot.stability_time_limit,
-          tolerance: tolerance || ((driver == :vips) ? 0.001 : nil),
-          # Deliberately LIVE (pinned by config_default_timing_test.rb):
-          # read at call time, never frozen into storage.
-          wait: Capybara.default_max_wait_time
-        }
-      end
-    end
-
-    # The old mattr_accessor surface, now delegating to the single storage.
-    # mattr_accessor used to define both singleton and instance accessors
-    # (the instance ones are what `include Capybara::Screenshot::Diff`
-    # picks up), so both are installed. root keeps its historical
-    # asymmetry -- readable everywhere, writable only at module level
-    # (it was mattr_reader plus a custom module-level writer) -- with the
-    # Pathname coercion now living in Config#root=.
-    SnapDiff::Config::MAPPING.each do |name, (mod, mattr)|
-      [mod, mod.singleton_class].each do |target|
-        target.define_method(mattr) { SnapDiff.config.public_send(name) }
-        next if name == :root && target == mod
-
-        target.define_method(:"#{mattr}=") { |value| SnapDiff.config.public_send(:"#{name}=", value) }
+        SnapDiff.config.default_options
       end
     end
   end

lib/snap_diff.rb

@@ -34,7 +34,7 @@ def self.assert_single_gem!(loaded_specs = Gem.loaded_specs)
 # stay resolvable (now with deprecation warnings) even in processes that
 # only ever require "snap_diff".
 # "capybara/dsl" is needed directly (not just transitively) so
-# `Capybara.default_max_wait_time` in Diff.default_options resolves even
+# `Capybara.default_max_wait_time` in Config#default_options resolves even
 # when "snap_diff" is required standalone (SnapDiffTest's
 # "standalone-loadable in a fresh process" regression test).
 require "capybara/dsl"
@@ -51,22 +51,28 @@ def self.assert_single_gem!(loaded_specs = Gem.loaded_specs)
 # shims (snap_diff/legacy_shims) that emit a deprecation warning once per
 # constant per process. See ADR-004 for the full migration plan.
 module SnapDiff
-  def self.compare(...)
-    Capybara::Screenshot::Diff.compare(...)
+  # Compare two images on disk with the configured defaults. Canonical home
+  # since ADR-008 step 7b; +Capybara::Screenshot::Diff.compare+ now forwards
+  # here rather than the other way round.
+  #
+  # Note the argument order swap: callers pass baseline first (reading
+  # "compare baseline against current"), Comparison takes current first.
+  def self.compare(baseline_path, current_path, **options)
+    Comparison.new(current_path, baseline_path, config.default_options.merge(options))
   end
 
-  # v1-style configuration: yields the two existing mattr_accessor holders
+  # v1-style configuration: yields the two legacy accessor holders
   # (+Capybara::Screenshot+, +Capybara::Screenshot::Diff+) exactly as
-  # +Capybara::Screenshot::Diff.configure+ always has. Kept byte-for-byte
-  # identical to +Diff.configure+ for existing callers migrating namespaces
-  # without changing call shape.
+  # +Capybara::Screenshot::Diff.configure+ always has -- and, since ADR-008
+  # step 7b, this is where that yield actually happens; Diff.configure
+  # forwards here. Both names stay identical in call shape.
   #
   #   SnapDiff.start do |screenshot, diff|
   #     screenshot.window_size = [1280, 1024]
   #     diff.tolerance = 0.0005
   #   end
-  def self.start(&block)
-    Capybara::Screenshot::Diff.configure(&block)
+  def self.start
+    yield Capybara::Screenshot, Capybara::Screenshot::Diff
   end
 
   # Forward-looking configuration: yields the single consolidated

lib/snap_diff/config.rb

@@ -42,7 +42,7 @@ module SnapDiff
   # +Rails.root+ / pwd) must never become lazy read-time defaults, memoized
   # or not. The one deliberately LIVE value, +default_options[:wait]+, is
   # not storage at all: it stays a method-body read of
-  # +Capybara.default_max_wait_time+ in +Diff.default_options+.
+  # +Capybara.default_max_wait_time+ in +#default_options+.
   class Config
     # config attr name => [legacy module, legacy accessor name].
     #
@@ -118,6 +118,53 @@ def initialize
     def root=(path)
       @root = Pathname(path).expand_path
     end
+
+    # --- Derived config (ADR-008 step 7b) -------------------------------
+    # Read-only values computed from the storage above. They used to live
+    # on the legacy modules; those now one-line forward here.
+
+    # ex +Capybara::Screenshot.active?+. The two +enabled+ settings are
+    # independent (see {MAPPING}): the Screenshot-side one wins whenever it
+    # was set at all, and only a nil there falls through to the Diff-side
+    # one.
+    def active?
+      screenshot_enabled || (screenshot_enabled.nil? && enabled)
+    end
+
+    # ex +Capybara::Screenshot.screenshot_area+: the save_path, optionally
+    # segmented per OS and per Capybara driver.
+    def screenshot_area
+      parts = [save_path]
+      parts << Os.name if add_os_path
+      parts << Capybara.current_driver.to_s if add_driver_path
+      File.join(*parts)
+    end
+
+    # ex +Capybara::Screenshot.screenshot_area_abs+.
+    def screenshot_area_abs
+      root / screenshot_area
+    end
+
+    # ex +Capybara::Screenshot::Diff.default_options+: the capture/compare
+    # defaults handed to {SnapDiff::Comparison}. Carries the one literal
+    # that is not a stored setting -- the vips tolerance floor.
+    def default_options
+      {
+        area_size_limit: area_size_limit,
+        color_distance_limit: color_distance_limit,
+        driver: driver,
+        screenshot_format: screenshot_format,
+        capybara_screenshot_options: capybara_screenshot_options,
+        perceptual_threshold: perceptual_threshold,
+        shift_distance_limit: shift_distance_limit,
+        skip_area: skip_area,
+        stability_time_limit: stability_time_limit,
+        tolerance: tolerance || ((driver == :vips) ? 0.001 : nil),
+        # Deliberately LIVE (pinned by config_default_timing_test.rb):
+        # read at call time, never frozen into storage.
+        wait: Capybara.default_max_wait_time
+      }
+    end
   end
 
   # Instantiated eagerly so the require-time defaults above are evaluated
@@ -129,4 +176,25 @@ def root=(path)
   def self.config
     @config
   end
+
+  # Installs the old mattr_accessor surface onto the legacy modules,
+  # delegating to the single storage above. mattr_accessor used to define
+  # both singleton and instance accessors (the instance ones are what
+  # `include Capybara::Screenshot::Diff` picks up), so both are installed.
+  # root keeps its historical asymmetry -- readable everywhere, writable
+  # only at module level (it was mattr_reader plus a custom module-level
+  # writer) -- with the Pathname coercion living in Config#root=.
+  #
+  # Generated here rather than in config_legacy.rb (ADR-008 step 7b) for
+  # the same reason legacy_shims.rb generates the legacy constants here:
+  # the generator is code, and the v1 trees must stay alias-only so 3.0 is
+  # a `git rm`. Same technique, same side of the fence.
+  Config::MAPPING.each do |name, (mod, mattr)|
+    [mod, mod.singleton_class].each do |target|
+      target.define_method(mattr) { SnapDiff.config.public_send(name) }
+      next if name == :root && target == mod
+
+      target.define_method(:"#{mattr}=") { |value| SnapDiff.config.public_send(:"#{name}=", value) }
+    end
+  end
 end

test/unit/legacy_tree_is_alias_only_test.rb

@@ -19,22 +19,22 @@ class LegacyTreeIsAliasOnlyTest < ActiveSupport::TestCase
     Dir[LIB.join("capybara_screenshot_diff/**/*.rb")]
   ).map { |path| Pathname.new(path) }.sort.freeze
 
-  # THE ALLOWLIST. Every entry is a file that legitimately still holds code,
-  # with the reason. This list is the honest state of the tree -- keep it in
-  # sync with ADR-008, and never add to it to make a red build green without
-  # first asking whether the code belongs in lib/snap_diff/ instead.
-  ALLOWED_WITH_CODE = {
-    # ADR-008 step 1 moved config STORAGE to SnapDiff::Config and generates
-    # the old accessor names here from Config::MAPPING (delegation, but via
-    # define_method). It also still holds DERIVED config logic that never
-    # moved: .active? precedence, .screenshot_area path assembly and
-    # .default_options (which carries one literal default, the vips
-    # tolerance 0.001). Narrowed below by pinning the method inventory, so
-    # new logic here still reds. AVAILABLE_DRIVERS also lives here on
-    # purpose (see #227: test_helper reads it at boot, image_compare_test
-    # stubs it) -- the gate does not push it out.
-    "capybara/screenshot/diff/config_legacy.rb" => "generates delegating accessors from Config::MAPPING; retains derived config logic (ADR-008 step 1)"
-  }.freeze
+  # THE ALLOWLIST -- and as of ADR-008 step 7b it is EMPTY: not one file in
+  # the v1 trees holds logic any more. config_legacy.rb was the last entry;
+  # step 7b moved its derived config (.active? precedence, .screenshot_area
+  # path assembly, .default_options incl. the vips tolerance literal) into
+  # SnapDiff::Config, and the Config::MAPPING accessor generator into
+  # snap_diff/config.rb alongside it, leaving only one-line forwarders.
+  #
+  # Keep it empty. Adding an entry back is a decision to keep behaviour on
+  # the v1 side of the 3.0 deletion, so it needs a written reason here AND
+  # an ADR-008 update -- never just to turn a red build green.
+  #
+  # (Diff::AVAILABLE_DRIVERS still lives in config_legacy.rb, but as a bare
+  # constant assignment it is alias-shaped and needs no exemption. It stays
+  # there on purpose -- see #227: test_helper reads it at boot and
+  # image_compare_test stubs it as the published no-drivers hook.)
+  ALLOWED_WITH_CODE = {}.freeze
 
   # Shapes that are pure compatibility plumbing rather than behaviour.
   ALIAS_SHAPES = /\A(
@@ -62,15 +62,13 @@ class LegacyTreeIsAliasOnlyTest < ActiveSupport::TestCase
     MSG
   end
 
-  test "config_legacy.rb keeps exactly its known set of methods" do
-    source = LIB.join("capybara/screenshot/diff/config_legacy.rb").read
-    defined_methods = source.scan(/^\s*def\s+(self\.)?([a-z_]\w*[?!]?)/).map { |receiver, name| "#{receiver}#{name}" }
-
-    assert_equal(
-      ["active?", "screenshot_area", "screenshot_area_abs", "self.compare", "self.configure", "self.default_options"],
-      defined_methods.sort,
-      "config_legacy.rb is allowlisted for the logic it already had, not for new logic -- move new methods to lib/snap_diff/"
-    )
+  # The pinned method inventory that used to narrow config_legacy.rb's
+  # allowlist entry is gone with the entry itself (ADR-008 step 7b): with
+  # nothing allowlisted, the general rule above already checks every `def`
+  # in the tree, config_legacy.rb's included.
+  test "the allowlist is empty, so nothing is exempt from the general rule" do
+    assert_empty ALLOWED_WITH_CODE,
+      "an exemption came back -- see the comment on ALLOWED_WITH_CODE before keeping it"
   end
 
   private

test/unit/snap_diff_config_test.rb

@@ -118,6 +118,41 @@ def config
     end
   end
 
+  # ADR-008 step 7b moved this precedence rule from
+  # Capybara::Screenshot.active? into Config#active?, and found it had no
+  # test at all: replacing the whole expression with a bare `enabled` kept
+  # all 529 unit tests green. The full truth table is pinned here, through
+  # both the canonical method and the legacy forwarder, so it cannot move
+  # again unnoticed.
+  #
+  # The rule: the Screenshot-side flag wins whenever it was set to anything
+  # at all; only a nil there falls through to the Diff-side flag.
+  ACTIVE_TRUTH_TABLE = [
+    [true, true, true],
+    [true, false, true],
+    [false, true, false],
+    [false, false, false],
+    [nil, true, true],
+    [nil, false, false]
+  ].freeze
+
+  test "active? gives Screenshot.enabled precedence and only falls through on nil" do
+    original_screenshot = Capybara::Screenshot.enabled
+    original_diff = Capybara::Screenshot::Diff.enabled
+
+    ACTIVE_TRUTH_TABLE.each do |screenshot_enabled, enabled, expected|
+      config.screenshot_enabled = screenshot_enabled
+      config.enabled = enabled
+      context = "screenshot_enabled=#{screenshot_enabled.inspect}, enabled=#{enabled.inspect}"
+
+      assert_equal expected, !!config.active?, "Config#active? with #{context}"
+      assert_equal expected, !!Capybara::Screenshot.active?, "Capybara::Screenshot.active? with #{context}"
+    end
+  ensure
+    Capybara::Screenshot.enabled = original_screenshot
+    Capybara::Screenshot::Diff.enabled = original_diff
+  end
+
   test "writing root through config round-trips through the same Pathname coercion" do
     original = Capybara::Screenshot.root