feat: deprecate chunky_png, shift_distance_limit and the driver abstraction (removed in 2.1) - #246
Conversation
…action (removed in 2.1)
2.0 is the transitional release: the contract is published before it is
enforced. The legacy-namespace half of that promise already warns. The
driver half -- everything 2.1 removes so that libvips becomes the only
backend -- warned about nothing. A user on `driver: :auto` without libvips
had no way to learn that 2.1 stops comparing for them.
Six warnings, each once per process, each silenceable through the existing
switches (`SnapDiff.silence_deprecations`, SNAP_DIFF_SILENCE_DEPRECATIONS):
- chunky_png SELECTED, from Utils.find_driver_class_for -- the one funnel
every selection surface ends up in (`driver:` per comparison,
SnapDiff.config.driver, the legacy Diff.driver=).
- `:auto` FALLING BACK to chunky_png because libvips is absent. Same funnel,
its own message and key: these users never asked for chunky_png.
- shift_distance_limit SET, from Config#shift_distance_limit= and from
Comparison#initialize (non-nil only -- default_options carries the key on
every comparison). One shared key, so it warns once whichever fires.
- Drivers.loaded, the documented custom-driver registration point.
- Drivers.available, i.e. driver detection.
- `include SnapDiff::Driver` by a class that is not one of the gem's own.
NOT warned on, and documented as silent: Drivers.for (the gem calls it for
every comparison, so warning there would fire on vips-only setups that
nothing here affects), detection itself (runs at load, before user code),
and the eager LOADED_DRIVERS / AVAILABLE_DRIVERS constant aliases (nothing
to hook). Each entry names the warning-capable equivalent instead.
The machinery is SnapDiff::Removal -- same channel, same discipline and the
same silencing switches as SnapDiff::Deprecation, in its own file: the
legacy shims and their deprecation channel are part of what the deletion
removes, while these call sites (utils, config, comparison, drivers) are
core files that outlive them and cannot depend on a doomed file.
SnapDiff.silence_deprecations moved there for the same reason -- it is the
one switch that silences both halves -- which is why the canonical-suite
gate no longer lists it as shim-only surface.
Two internal seams keep the gem from warning at itself: Drivers.registry
(the unannounced registry the gem reads) and reading the
AVAILABLE_DRIVERS constant directly instead of .available. test_helper
suppresses the channel the same way it already suppresses the migration
notice: this suite runs its whole matrix on chunky_png by design.
Evidence: 13 subprocess probes ("once per process" cannot be measured
inside one long-lived suite), mutation-tested -- dropping the dedup goes 9
red, dropping the silence check 2 red, routing an internal read back
through .loaded 5 red, unscoping the mixin hook 5 red, warning on the
presence of shift_distance_limit rather than a value 4 red.
rake test:unit 585/0, rake test 613/0/1, rake test:canonical 482/0/1,
standardrb clean.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Warning Review limit reached
Next review available in: 44 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 (16)
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 |
Reviewer's GuideIntroduces a dedicated SnapDiff::Removal warning channel for features that will be removed in 2.1 (chunky_png driver, shift_distance_limit, and the driver abstraction) and wires it into core code paths, while updating docs and tests to reflect and verify the new removal warnings and shared silencing behavior. Sequence diagram for driver removal warningssequenceDiagram
participant User
participant Utils
participant Removal
participant Drivers
participant Comparison
User->>Comparison: initialize(image_path, base_image_path, options)
Comparison->>Removal: warn_once(:shift_distance_limit, ...)
Comparison->>Drivers: for(driver_options)
Drivers->>Utils: find_driver_class_for(driver)
alt driver == :chunky_png
Utils->>Removal: warn_once(:chunky_png, ...)
else driver == :auto and chunky_png selected
Utils->>Removal: warn_once(:chunky_png_auto, ...)
end
Utils->>Drivers: registry[driver]
Removal-->>User: Kernel.warn once per subject
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
beta3 fixed the canonical entry points and shipped almost none of the behaviour. beta4 is the prerelease the 2.0.0 entry actually describes: the four green-suite bugs (#254 #255 #256 #258), the accept workflow (#259), the legible failure message (#264), and the deprecation warnings that make 2.1's removals visible (#246 #263). - `lib/snap_diff/version.rb` -> 2.0.0.beta4. Nothing else holds a version; the gemspec, the legacy version file and the mirror gemspec all read it. Verified with the release workflow's own guard command: `ruby -I lib -r capybara/screenshot/diff/version -e "puts Capybara::Screenshot::Diff::VERSION"` => 2.0.0.beta4 - CHANGELOG: a `[v2.0.0.beta4]` section written as the delta from beta3, plus the record-modes PLACEHOLDER filled from #259 now that it has shipped. `grep -n PLACEHOLDER CHANGELOG.md` returns nothing. - Install snippets moved beta3 -> beta4 in README, docs/UPGRADING.md and docs/migration-guide.md. They stay PINNED: `~> 2.0` resolves to nothing while only prereleases exist, so unpinning belongs to 2.0.0 final, not here. The notes name the #272 caveat explicitly. Removing `skip_area`'s implicit stabilization wait (10.012 s -> 0.009 s measured) means a selector not yet in the DOM now yields no mask, silently, where it previously resolved after the wait. #277's run-level tally shipped in the same beta as the replacement signal, and the notes say so rather than leaving it to be discovered. Gates: `rake test:unit` 720 runs / 2124 assertions / 0 failures / 0 skips under CI=true on 4.0.6, `standardrb lib test` clean over 161 files, and `gem build` produces capybara-screenshot-diff-2.0.0.beta4.gem (93 files, 13 docs, RELEASE_PREP correctly excluded).
|
Big up-front caveat: I'm an outsider looking in to this project. I'm really only in the very early stage of evaluating and using it in my own projects. I'm only currently using it indirectly for comparing svg -> png images that my app generates, not even for capybara screenshots! This decision doesn't (currently) affect me! With that out of the way: I'm curious about why the Because having some sort of image comparison driver abstraction seems like a very obvious and useful extension point. Although ChunkyPng is slower today and isn't being actively maintained, there are still risks associated with bringing in libvips1 that some projects may wish to avoid, even just for dev/test dependencies. And with the rapid improvements to YJIT and ZJIT2, or compatibility improvements to alternate runtimes like JRuby or TruffleRuby, it's far less obvious that libvips will be the "winner" a couple of years from now. Also, dropping support for chunky_png doesn't require dropping the driver API entirely. The existence of a variety of image diff tools today suggests that it may be useful to use different comparison tools and/or algorithms, without needing to fork the project or add a bunch of new config options3. All of that said, it also makes sense that this And anyway, I'm not maintaining this project nor am I volunteering to help, so... maybe I shouldn't be second guessing this decision. I'm only commenting because I'm actively evaluating snap-diff right now4. Please take my words with a huge grain of salt! 😉 Footnotes
|
|
@nevans thanks for checking the code. Could you elaborate/clarify, shorter and simpler. I’m afraid that I did not get from the comment what exactly do you want. Also could you share your use cases. How do you use the gem. The reason why I want to drop, because nobody used it, at least what I know. But we spend energy to support it. And when we will get a new feature request or confirm demand we will be able to add new driver in no time. |
2 participants
2.0 is the transitional release: the contract is published before it is enforced. The
legacy-namespace half of that already warns. The driver half — everything 2.1 removes so
that libvips becomes the only backend — warned about nothing. This adds it.
What now warns (once per process, per subject)
driver: :chunky_png,SnapDiff.config.driver =, the legacyDiff.driver =Utils.find_driver_class_for— the one funnel every selection surface ends up indriver: :autofalls back to chunky_png because libvips is absentshift_distance_limitis set — globally or per comparisonConfig#shift_distance_limit=andComparison#initialize(non-nil only), one shared dedup keySnapDiff::Drivers.loadedis read — the documented custom-driver registration pointDrivers.loadedSnapDiff::Drivers.availableis readDrivers.availableinclude SnapDiff::DriverDriver.included, scoped off the gem's own two driversNot warned on, deliberately:
Drivers.for(the gem calls it for every comparison — warningthere would fire on vips-only setups that nothing in this list affects), driver detection
(
Drivers.detect_available/Utils.detect_available_drivers, which run at load before anyuser code), and the eager legacy constant aliases
LOADED_DRIVERS/AVAILABLE_DRIVERS(plain constants, nothing to hook). All three are documented as silent in
docs/drivers.mdand
docs/UPGRADING.md, with the warning-capable name to use instead.Machinery
SnapDiff::Removal(lib/snap_diff/removal.rb) — same shape, same channel (Kernel#warn),same silencing switches (
SnapDiff.silence_deprecations/SNAP_DIFF_SILENCE_DEPRECATIONS)as
SnapDiff::Deprecation, but its own file: the legacy shims and their deprecation channelare part of what gets deleted, while these call sites (utils, config, comparison, drivers) are
core files that outlive them and cannot depend on a doomed file.
SnapDiff.silence_deprecationsmoved there for the same reason — it is the one switch that silences both halves.
Two internal seams so the gem never warns at itself:
Drivers.registry(the unannouncedregistry the gem reads) and reading
Drivers::AVAILABLE_DRIVERSdirectly instead of.available.Evidence
test/unit/removed_in_2_1_deprecation_test.rb— 13 subprocess probes ("once per process" cannotbe measured inside one long-lived suite). Mutation-tested: dropping the dedup → 9 red; dropping
the silence check → 2 red; routing an internal read back through
.loaded→ 5 red; unscoping themixin hook → 5 red; warning on the presence of
shift_distance_limitrather than a real value →4 red.
rake test:unit585 runs / 0 failures (baseline 572)rake test:canonical482 runs / 0 failures / 1 skipstandardrbcleanDocs
docs/drivers.md(what goes, what replaces it, libvips becomes required, and that customdrivers have no migration path),
docs/UPGRADING.md(third warning class next to the twonamespace ones),
docs/configuration.md(shift_distance_limit),docs/snapdiff.md(thecustom-driver page a driver author actually lands on).
Summary by Sourcery
Warn users about driver-related features removed in 2.1 and guide them toward libvips before the transition.
Enhancements:
Documentation:
Tests: