Support Ruby 3.0 kwarg-handling within .delay API
#7
Conversation
|
Needs somebody from @effron and @samandmoore to claim domain review Use the shovel operator to claim, e.g.:
|
| def send_at(time, method, *args) | ||
| warn '[DEPRECATION] `object.send_at(time, :method)` is deprecated. Use `object.delay(:run_at => time).method' | ||
| __delay__(run_at: time).__send__(method, *args) | ||
| end |
There was a problem hiding this comment.
Removing these -- they were already deprecated prior to our fork.
|
<<domainLGTM |
|
Needs somebody from @effron and @samandmoore to claim platform review Use the shovel operator to claim, e.g.:
|
|
|
||
| if defined?(ActionMailer::Parameterized::Mailer) | ||
| describe ActionMailer::Parameterized::Mailer do | ||
| describe 'delay' do |
There was a problem hiding this comment.
I didn't actually add this section -- I just reorganized the file a little.
|
<<platformlgtm |
| object.send(method_name, *args) if object | ||
| return unless object | ||
|
|
||
| if kwargs.nil? || (RUBY_VERSION < '2.7' && kwargs.empty?) |
There was a problem hiding this comment.
There's a proposed PR in the one of the ancestor repos, but for this PR I took a very different strategy, because we want to both maintain Ruby 2.6 support and support the Ruby 3 case where both positional and keyword arguments are present. Holding onto kwargs separately resolves edge cases around argument auto-merging behaviors, but then on Ruby 2.6 we need to fall back in the **{} case (since on 2.6 and below that causes {} to get passed as a positional argument).
There was a problem hiding this comment.
But I think that this should resolve collectiveidea/delayed_job#1134 and might be worth upstreaming, if someone gets a chance.
smudge
changed the title
.delay API.delay API
|
Approved! 🍩 👍 ✨ |
smudge
changed the title
.delay API.delay API
|
Approved! 🎏 🎁 🌯 |
|
This PR requires additional review because of new changes Please get another domain review from @effron, or another reviewer with write access if unavailable. |
|
nice. glad you tested it :) domainltm |
|
ugh domainlgtm curse this keyboard! |
|
This PR requires additional review because of new changes Please get another domain review from one of @effron, @samandmoore, or another reviewer with write access if unavailable. |
|
Sorry for the review churn! I've been testing this in the context of our largest Rails monorepo, and seem to have ironed out the kinks, so I'd like to cut a new version all in one go. |
|
domainlgtm |
| object.send(method_name, *args) if object | ||
| return unless object | ||
|
|
||
| if kwargs.nil? || (RUBY_VERSION < '2.7' && kwargs.empty?) |
There was a problem hiding this comment.
You can check if the callee uses kwargs. First get the method object with object.method(method_name) and check its parameter list for :key, :keyreq or :keyrest. (From experience, Rails Mailer magical methods end up with a single :rest argument that you'd need to account for as well.)
There was a problem hiding this comment.
In this case, I don't think we want it to actually care about the arity of the method it's about to call -- only the arity that was provided to it by the enqueuing call. (For example, if I do obj.delay.something(test: 123) on a method that does not accept kwargs, I want to ensure that the job raises an error and tells me that my arguments are wrong.)
So the fix here is really just to prevent **{} from breaking on Ruby 2.6 (because trying to pass that as a kwarg results in it passing {} as a positional arg). This quirk was fixed in 2.7.
|
domainLGTM |
|
Approved! 💯 🍕 💅 |
/domain @effron @samandmoore
/platform @effron @samandmoore
Resolves #6. While we already fully support Ruby 3.0 kwargs via ActiveJob, the legacy
delayandhandle_asynchronouslyAPIs did not support the new separation of positional and keyword arguments introduced in 2.7 and enforced in 3.0.This means that methods accepting both positional and keyword arguments (e.g.
def foo(a, b:)) would fail with awrong number of arguments (given 2, expected 1; required keyword:)error on Ruby 3. In order to resolve this, this PR changesDelayed::PerformableMethodto handle kwargs separately from args, with a backwards compatibility check for any Ruby 2.6 methods that do not accept keyword arguments. It should also support jobs that were enqueued by the priordelayedgem version (where the newkwargsaccessor would be nil, andargscontains the kwargs as its last item).