refactor: default reporter, Comparison::Images, driver registry get SnapDiff homes (ADR-008 steps 4+5) - #227
Merged
Conversation
Pure git mv so the rename survives with history; the rewrap to SnapDiff::Reporters::Default and the old-path forwarder land in the next commit.
…008 step 4) The last old-namespace CLASS dependency in the core: comparison.rb now requires and constructs SnapDiff::Reporters::Default. The old constant Capybara::Screenshot::Diff::Reporters::Default joins the legacy_shims mapping (lazy, warn-once, same-object -- pinned by the MAPPING-driven forwarding and deprecation tests, now 30 pairs). The old file path stays requirable as a forwarder. Test edits are canonical-name migrations only (per the #221 policy -- the raise-on-deprecation guard would trip on the old name otherwise): default_test.rb and image_compare_test.rb reference SnapDiff::Reporters::Default; no expectations changed.
…ADR-008 step 5a) Resolves the Comparison name collision: the images-holder struct that lived at Capybara::Screenshot::Diff::Comparison (blocked from a SnapDiff home because SnapDiff::Comparison is the comparator class) is now nested inside the comparator as SnapDiff::Comparison::Images. The old constant joins the legacy_shims mapping (lazy, warn-once, same-object -- 31 pairs now). comparison.rb constructs Images directly; no old-namespace class reference remains in the compare path. Test edits are canonical-name migrations only (per the #221 policy; the raise-on-deprecation guard would otherwise trip): the struct constructions in driver_contract_tests, image_preprocessor_test, annotation_service_test and default_test now spell the canonical name. No expectations changed.
SnapDiff::Drivers.loaded is now the canonical driver-class cache (ex Capybara::Screenshot::Diff::LOADED_DRIVERS, which comparison.rb no longer defines); the old constant stays as an EAGER same-object alias in legacy_shims because user code registers custom drivers by mutating the hash in place -- a lazy copy would silently drop registrations, and warning on a supported surface would be noise. SnapDiff::Drivers.available is the canonical reader for the detected drivers list; the value itself stays on the eager Capybara::Screenshot::Diff::AVAILABLE_DRIVERS constant (config_legacy detection moment unchanged, and image_compare_test pins that constant as the stub point for the no-drivers error path). Utils' find_driver_class_for now reads both registries through the canonical accessors, removing the last old-namespace reads from the core compare path. Guard tests pin same-object identity and legacy-write -> canonical-read visibility.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Warning Review limit reached
Next review available in: 31 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Reviewer's GuideThis PR finishes ADR-008 steps 4 and 5 by moving the default reporter and comparison images struct into the SnapDiff namespace, and by introducing canonical driver registry accessors while keeping legacy Capybara::Screenshot::Diff constants working via shims and eager aliases. Comparison.rb no longer depends on legacy namespaces; tests are updated to use canonical names and new shims/aliasing are covered by namespace forwarding and driver registry tests. Sequence diagram for canonical comparison and driver lookupsequenceDiagram
participant Caller
participant Comparison as SnapDiff::Comparison
participant Utils as SnapDiff::Utils
participant Drivers as SnapDiff::Drivers
participant Driver
participant Reporter as SnapDiff::Reporters::Default
Caller->>Comparison: load_comparison(base_path, new_path, options)
Comparison->>Utils: find_driver_class_for(driver)
Utils->>Drivers: available()
Utils->>Drivers: loaded[driver]
Drivers-->>Utils: driver class
Utils-->>Comparison: driver class
Comparison->>Driver: load_images(base_path, new_path)
Driver-->>Comparison: base_img, new_img
Comparison->>Comparison: Images.new(...)
Caller->>Comparison: reporter()
Comparison->>Reporter: new(current_difference)
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This was referenced Aug 22, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ADR-008 steps 4 and 5 in one PR: both center on
lib/snap_diff/comparison.rb, which now contains zero old-namespace references.Step 4 —
SnapDiff::Reporters::Defaultgit mv lib/capybara/screenshot/diff/reporters/default.rb lib/snap_diff/reporters/default.rb(mv-then-rewrap commit pair, history preserved), rewrapped asSnapDiff::Reporters::Default.comparison.rbrequires and constructs the canonical name — this was the last old-namespace CLASS dependency in the core.Capybara::Screenshot::Diff::Reporters::Defaultjoins the legacy_shims mapping: lazy, warn-once, same-object, like its peers. Old file path stays requirable as a forwarder.Step 5a —
SnapDiff::Comparison::ImagesThe images-holder struct
Capybara::Screenshot::Diff::Comparison(blocked from a SnapDiff home becauseSnapDiff::Comparisonis the comparator class) is now nested inside the comparator asSnapDiff::Comparison::Images. Old constant joins the legacy_shims mapping (lazy, warn-once).Step 5b — driver registry
SnapDiff::Drivers.loadedis the canonical driver-class cache (exLOADED_DRIVERS, no longer defined bycomparison.rb). The old constant is an EAGER same-object alias (assigned in legacy_shims): user code registers custom drivers by mutating the hash in place (LOADED_DRIVERS[:foo] = ...), so a lazy warn-once shim would either copy (silently dropping registrations) or warn on a supported surface.SnapDiff::Drivers.availableis the canonical reader; the value stays on the eagerCapybara::Screenshot::Diff::AVAILABLE_DRIVERSconstant (detection moment inconfig_legacy.rbunchanged, andimage_compare_testpins that constant as thestub_constpoint for the no-drivers error path — a canonical-side cached copy would break that stub).Utils.find_driver_class_fornow reads both registries through the canonical accessors.Test edits (canonical-name migrations only, per the #221 policy)
No expectations changed anywhere — these tests construct/assert the same objects under their canonical spellings (the suite's raise-on-deprecation guard would otherwise trip on the lazy shims):
test/support/driver_contract_tests.rb— 4 struct constructions →SnapDiff::Comparison::Images(the test: migrate suite to SnapDiff canonical names #221-documented edit).test/unit/image_preprocessor_test.rb,test/unit/annotation_service_test.rb,test/unit/reporters/default_test.rb— bareComparison.newstruct constructions resolved lexically through the old nesting; same migration.test/unit/reporters/default_test.rb,test/unit/image_compare_test.rb—Reporters::Defaultreferences →SnapDiff::Reporters::Default.test/unit/namespace_forwarding_test.rb— MAPPING gains the two new lazy pairs (29 → 31, count test updated; forwarding + warn-once tests auto-generate from MAPPING), plus two new registry guards: same-object identity for.loaded/.availablevs the old constants, and a write-via-old-name / read-via-canonical mutation probe.Evidence
Baseline at 4288edb:
rake test:unit518 runs / 0 failures. After: 524 runs / 0 failures (+6: 2×2 MAPPING-generated for the new shim pairs, 2 registry guards). Fullrake test: 557 runs, 0 failures, 6 skips (skips are the usual driver/browser-dependent ones).standardrb: clean.Mutation checks (each applied, observed red, reverted):
Comparison:→SnapDiff::ComparisonResult) →NamespaceForwardingTest...Comparison_forwards_to_SnapDiff::Comparison::Imagesfails onassert_same.LOADED_DRIVERS = SnapDiff::Drivers.loaded.dup) → both registry guards fail (same-object + mutation probe).comparison.rb(Reporters::Defalt) → 5 failures + 136 errors across the suite (ImageCompareTest, driver tests, ScreenshotAssertionTest, snap_diff_test, ...) — every path that reaches#reporter/#processed.🤖 Generated with Claude Code
Summary by Sourcery
Complete ADR-008 namespace migration for the default reporter, comparison images, and driver registries while maintaining backward compatibility.
Enhancements:
Documentation:
Tests: