Skip to content

Conversation

@mdkent
Copy link
Contributor

@mdkent mdkent commented Sep 30, 2025

This is the resque-pool side of resque/resque#1920. See the notes there.

@nevans
Copy link
Collaborator

nevans commented Oct 2, 2025

I like these PRs. Please see my comments on resque/resque#1920. 🙂

For this PR, we need to ensure backward compatibility. (I'm not sure why CI didn't run the test suite.) We could reflect on method arity (👎🏻) or parameter names (better, but verbose), or we could check for the existence of specific ENV vars (👎🏻 extra coupling), or we could check Resque::VERSION (but that won't work until the next gem version is released). So I guess my vote is checking for parameter names.

I'd prefer if Resque or Resque::Worker had a #work_with_env_defaults method (maybe there's a better name?). Or maybe #work could automatically applied its own ENV defaults? Maybe there's already a method that does that and I'm being lazy by not searching the source code right now? That would reduce this cross-gem duplication. And there were a method in Resque or Resque::Worker that does that, we could use Resque::Worker.respond_to?(:work_with_env_defaults) for backward compatibility. I'd much prefer that over the other approaches.

@mdkent mdkent force-pushed the dynamic-work-interval branch 3 times, most recently from 89171a3 to d403b1b Compare November 28, 2025 22:48
@mdkent
Copy link
Contributor Author

mdkent commented Nov 28, 2025

Went for the simplest option here in just checking the number of params.

Tested okay here on our staging setup. Tried both the updated resque from resque/resque#1919 and the version from master and both are happy.

@nevans
Copy link
Collaborator

nevans commented Dec 2, 2025

@mdkent I've fixed CI on main. Can you go ahead and rebase or merge to include those changes?

Once the matching API in resque/resque#1920 is approved, I think we can go ahead an merge this.

While also maintaining backwards compatibility. See
resque/resque#1920.
@mdkent mdkent force-pushed the dynamic-work-interval branch from d403b1b to 1646491 Compare December 3, 2025 18:11
@mdkent
Copy link
Contributor Author

mdkent commented Dec 3, 2025

All set!

@nevans nevans merged commit 04dc061 into resque:main Dec 5, 2025
210 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants