Repository navigation
Declare dependencies and announce 4.0's breaking changes (3.7.0) - #47
Conversation
Declared at railties' own floor so nothing installable is excluded. Patches::Patch subclasses ActiveRecord::Base and Patches::Base#execute calls ActiveRecord::Base.connection, but only railties was declared.
ActiveSupport::Deprecation's singleton was deprecated in Rails 7.1 and removed in 8.0, so the gem carries its own instance. Host applications can silence it with Patches.deprecator.behavior = :silence.
Warns once at load when Ruby is below 3.2 or Rails below 7.2, the floors 4.0 is expected to set, and when the pre-6.3 Sidekiq fallback is taken. Those floors are higher than the range CI verifies, so the notice reaches consumers on versions that still pass. Nothing changes behaviour.
None were set, so rubygems.org shows values carried over from older releases - 3.6.3 still lists source_code_uri under the old jobready org.
It published on release: published with a GEM_HOST_API_KEY, never ran in four years, and would now duplicate the tag-driven release. RELEASING.md covers the current process.
The shared workflow this came from is also used by internal gems, which do publish to GitHub Packages. patches is public and publishes only to RubyGems.org, so nothing here uses the scope.
The development container was pinned to ruby:2.3, which cannot satisfy the current Gemfile, and had not been touched since 2018. The Ruby version is now a build argument and gem versions pass through from the shell, so any cell of the matrix is reproducible in it. .dockerignore keeps the uncommitted Gemfile.lock out of the image, which otherwise pinned it to whatever the host last resolved. Adds a devcontainer and reuses the compose service. Points at docker-compose.yml rather than defining a second image, so the Ruby version and env passthrough stay in one place. overrideCommand is needed because the service's own command runs the specs and would exit.
The package carried the Dockerfile, .devcontainer, .github, bin, Rakefile, RELEASING.md and a 52K decorative image - 42 files. Now 27: lib, the install migration, docs/usage.md and the top-level docs.
setup-ruby's bundler-cache sets the bundle path to vendor/bundle, so a developer copying that locally would otherwise see it untracked.
It opened with two dead Codeclimate badges - the repo moved to SonarCloud in #39 and SonarQube in #43 - and a Version 2.0 breaking-change notice from six years ago, then version tables before Installation. Now: what it is, how to install including the migration, a worked example, then the reference sections. The deploy.rb note it dropped is already in docs/usage.md.
Nothing here has deployed with Capistrano for a long time, but the gem is public, so the task warns in 3.7.0 and goes in 4.0. It stays opt-in and loads nothing unless a Capfile requires it. Nothing loaded lib/patches/capistrano.rb, so SimpleCov never saw it and SonarQube counted its lines as uncovered new code - 86.70% against a 95% gate. Loading it the way a Capfile does also exercises the deprecation on the real path rather than only through the method.
Patches requires slack-notifier unconditionally while guarding sends with defined?(Slack), a guard that can never be false - Slack was meant to be optional like Sidekiq and never finished. The warning fires only when config.use_slack is set, once, at configuration time.
e2d9917 to
74d3e8c
Compare
patches.gemspec requires patches/version, so Bundler loads it under bundle exec before spec_helper starts SimpleCov - its lines can never be recorded as covered, and Sonar counted the version bump as uncovered new code (93.30% against a 95% gate). Still analysed, just not measured.
74d3e8c to
9c062f9
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Load-time warnings cannot be configured reliably, repeated Slack configuration duplicates notices, and the documented container matrix command does not install the requested bundle.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Prepares Patches 3.7.0 by declaring ActiveRecord, introducing 4.0 deprecations, modernizing development tooling, and streamlining packaging and releases.
Changes:
- Adds configurable deprecation warnings and coverage.
- Updates gem metadata, dependencies, packaging, and version.
- Refreshes documentation, containers, and workflows.
File summaries
| File | Description |
|---|---|
.dockerignore |
Excludes generated development files. |
.devcontainer/devcontainer.json |
Adds VS Code container configuration. |
.github/workflows/publish.yml |
Removes obsolete publishing workflow. |
.github/workflows/release.yml |
Drops unused package permission. |
.github/workflows/specs.yml |
Excludes version file from coverage. |
.gitignore |
Ignores vendored dependencies. |
CHANGELOG.md |
Documents the 3.7.0 release. |
Dockerfile |
Modernizes the development image. |
README.md |
Expands usage and development guidance. |
RELEASING.md |
Removes obsolete release rationale. |
docker-compose.yml |
Adds matrix-related environment settings. |
docs/usage.md |
Documents upcoming removals. |
lib/patches.rb |
Emits version-support warnings. |
lib/patches/capistrano.rb |
Warns about Capistrano removal. |
lib/patches/config.rb |
Warns when Slack is enabled. |
lib/patches/deprecation.rb |
Defines the gem-owned deprecator. |
lib/patches/sidekiq_job.rb |
Warns on the legacy Sidekiq fallback. |
lib/patches/version.rb |
Bumps the version to 3.7.0. |
patches.gemspec |
Adds metadata, ActiveRecord, and package filtering. |
spec/capistrano_spec.rb |
Tests Capistrano behavior. |
spec/deprecation_spec.rb |
Tests announced deprecations. |
spec/sidekiq/sidekiq_job_spec.rb |
Tests Sidekiq warnings. |
spec/spec_helper.rb |
Silences unrelated test warnings. |
Review details
- Files reviewed: 22/23 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
msxavi
left a comment
msxavi
left a comment
There was a problem hiding this comment.
Automated review: security, performance, and best-practices pass. No security or performance issues found; a few convention/consistency notes inline.
| require "patches/notifier" | ||
| require "patches/worker" if defined?(Sidekiq) | ||
|
|
||
| Patches.warn_about_versions_dropped_in_4_0 |
There was a problem hiding this comment.
This fires unconditionally at require 'patches' time (via Bundler.require), which runs before a host app's config/initializers/*.rb. The README's documented silencing mechanism (Patches.deprecator.behavior = :silence) is set in an initializer, so it can't suppress this particular warning — it will already have printed by the time the initializer runs. The other three deprecations (capistrano, config, etc.) fire lazily at call time and are correctly silenceable. Worth deferring this one similarly, e.g. by hooking it to a lazy trigger point instead of module load.
There was a problem hiding this comment.
[AI] Correct, and this was a real design error rather than a nitpick: the README documents Patches.deprecator.behavior = :silence as the way to quiet these notices, while this one fired during Bundler.require — before any initializer could run. The documented mechanism could not work for it.
Fixed in 0eec058 by deferring it to the engine:
initializer 'patches.version_deprecations', after: :load_config_initializers do
Patches.warn_about_versions_dropped_in_4_0
endrequire "patches" now emits nothing — verified: 0 deprecation lines on a Ruby 3.1 / Rails 7.1 boot that previously printed both notices. Specs assert the initializer is registered, that it is ordered after: :load_config_initializers, that it warns when run, and that requiring the gem alone stays silent.
As Copilot noted on the same line, this also removes the order dependence: inside an initializer Rails::VERSION is always loaded, so the Rails check can no longer be skipped silently.
(Edited: I originally flagged that a non-Rails consumer would no longer get the notice. That was not worth raising — railties and activerecord are hard dependencies and the gem ships an engine, so every real consumer is a Rails application. The defined?(Rails) guards in lib/ are defensive leftovers; one of them read defined?(:Rails), which is always truthy, until 3.6.3 — proof nobody had ever exercised that path. Removing them is queued for 4.0.)
| spec.files = `git ls-files -z`.split("\x0").reject { |f| f.match(%r{^(test|spec|features)/}) } | ||
| spec.metadata = { | ||
| "source_code_uri" => spec.homepage, | ||
| "changelog_uri" => "#{spec.homepage}/blob/develop/CHANGELOG.md", |
There was a problem hiding this comment.
changelog_uri (and documentation_uri below) point at the floating develop branch rather than a tagged release ref. Since releases are tagged (v2.0.0, v3.6.3, ...) and CHANGELOG.md/docs/usage.md keep changing on develop, a user who installs gem version X and follows this link from RubyGems can land on content that has already diverged from what they installed. Consider pointing these at the release tag, e.g. #{spec.homepage}/blob/v#{Patches::VERSION}/....
There was a problem hiding this comment.
[AI] Good catch — fixed in 0eec058. Both now interpolate the version:
"changelog_uri" => "#{spec.homepage}/blob/v#{Patches::VERSION}/CHANGELOG.md",
"documentation_uri" => "#{spec.homepage}/blob/v#{Patches::VERSION}/docs/usage.md"Confirmed in the built gem: changelog_uri: https://github.com/rdytech/patches/blob/v3.7.0/CHANGELOG.md. The gemspec already requires patches/version for spec.version, so nothing new is loaded.
source_code_uri stays on the repository root deliberately — it is the project, not a snapshot. bug_tracker_uri likewise.
Worth noting the links 404 until the tag is pushed, which for this flow is moments after the gem publishes (rake release tags, then pushes). Since rubygems.org only ever shows released versions, nobody should hit a dead link.
| .bundle | ||
| coverage | ||
| test.db | ||
| *.gem |
There was a problem hiding this comment.
This PR also adds /vendor/ to .gitignore (for a locally vendored bundle path), but .dockerignore isn't updated to match. If a dev vendors gems locally (bundle config set --local path vendor/bundle) and then runs docker compose build, the full vendor/bundle tree gets sent as build context and copied into the image — unnecessary bloat given gems are already stored via the compose volume. Consider adding vendor here too.
There was a problem hiding this comment.
[AI] Fixed in 0eec058 — vendor added to .dockerignore, so the two files agree.
You are right about the mechanism: the build context is what gets sent, and COPY . /app/ would then bake a vendored tree into the image, on top of the gems the compose volume already holds at /usr/local/bundle.
For what it is worth, the /vendor/ entry in .gitignore is itself belt-and-braces — nothing in the committed config vendors gems locally, and ruby/setup-ruby only does it inside an ephemeral runner workspace. But if someone does adopt the habit, both files now cover it.
- The Ruby/Rails notice moves to a Rails initializer, after the application's own, so Patches.deprecator.behavior can silence it. At require time it ran during Bundler.require, before any initializer, making the documented mechanism useless. - The Slack notice fires once per configuration rather than on every truthy assignment, so a reloading initializer does not repeat it. - changelog_uri and documentation_uri point at the release tag rather than develop, so a link from RubyGems matches the version installed. - .dockerignore excludes vendor, matching the .gitignore entry, so a locally vendored bundle is not sent as build context. - The container recipe for a non-default cell installs the gems first; the image is built with the defaults, so it could not have worked as written. - CHANGELOG named Ruby 3.0 / Rails 7.1 as 4.0's floors, contradicting the implementation. Lost in an earlier rebase.
The once-per-configuration guard added for review feedback made these order-dependent: other spec files set use_slack on the shared configuration, so the notice had already fired and the examples saw no message. One CI cell caught it.
|
0 New Issues
0 Fixed Issues
0 Accepted Issues
Additive only: no public API changes and no version constraint moves, so anything
that installed 3.6.x installs this. Its job is to declare dependencies the gem
always relied on, and to announce every breaking change 4.0 will make a release
ahead of time.
Deprecations announced for 4.0
require 'patches/capistrano'rake patches:runfrom your deployment processslack-notifiergem 'slack-notifier'to your Gemfileconfig.use_slackis setSidekiq::Workerfallback is takenThose two floors are deliberately higher than the range CI verifies — Ruby 3.0
and Rails 7.1 still pass — so consumers below them hear about it now rather than
at upgrade time.
All four go through
Patches.deprecator, a gem-ownedActiveSupport::Deprecationinstance rather than the singleton Rails 7.1 deprecated and 8.0 removed. A host
can silence or redirect it:
Declared dependencies
activerecord, atrailties' own>= 3.2floor so nothing installable isexcluded.
Patches::PatchsubclassesActiveRecord::BaseandPatches::Base#executecallsActiveRecord::Base.connection, yet onlyrailtieshad ever been declared.source_code_uriashttp://github.com/jobready/patches, from before the orgwas renamed.
Housekeeping
Dockerfile,docker-compose.yml,.devcontainer/,.github/,bin/,Rakefile,RELEASING.mdand a decorative image were all being packaged.FROM ruby:2.3, untouchedsince 2018, and could not satisfy the current Gemfile — while the README told
people to use it. The Ruby version is now a build argument and gem versions
pass through from the shell, so any cell of the matrix is reproducible in it. A
.devcontainer/referencing the same compose service comes with it, so there isone definition of the environment rather than two.
publish.ymlremoved. It had never run in the four years since it wasadded, and would now attempt a duplicate push if a GitHub Release were created
after a tag. Releases come from
release.yml— seeRELEASING.md.packages: writedropped from the release workflow; nothing here publishesto GitHub Packages.
never mentioned, and drops two dead Codeclimate badges (the repo moved to
SonarCloud in Replace codeclimate with sonarcloud #39 and SonarQube in [RST-13] Migrate to Sonarqube #43) plus a
Version 2.0notice from 2019.Verification
CI is authoritative here: 17 Ruby/Rails cells (Ruby 3.0–4.0 against Rails
7.1–8.1) plus a leg for each Sidekiq state — absent, 6.5, and 7.x/8.x with strict
arguments both on and off.
Locally the same 23 configurations pass, and the deprecations were exercised from
a consumer's point of view rather than only through specs: on Ruby 3.1.6 /
Rails 7.1.6 both floor notices fire at
require "patches", and enablingconfig.use_slackwarns whileconfig.use_sidekiqstays silent.Reviewing
Thirteen commits, one idea each.
Release 3.7.0last, holding only the versionbump and changelog.
🤖 Generated with Claude Code