Fix type of retry option: retry can be boolean or a number
#49
Conversation
Leave it as a JsonValue for now for better compatibility
|
Thanks for the PR. Good find! I think |
|
@Antti would you mind allowing maintainer push to your fork? I refactored from JsonValue (which was a very helpful refresher so thank you for the PR) to a new RetryOpts type with a backwards compatible retry() method that now supports: opts()
// old way
.retry(true).retry(false)
// now usize is supported
.retry(42)
// and the new enum is available too
.retry(RetryOpts::Yes).retry(RetryOpts::Never).retry(RetryOpts::Max(42)I have a branch up here that I can open a new PR with but would love your thoughts: https://github.com/film42/sidekiq-rs/compare/gt/retry_opts?expand=1 I would like to add a few tests but I'm on a flight at the moment and I'll be out of internet range soon so wanted to get this up for the moment. |
This changes JsonValue to RetryOpts which is an enum for Yes, Never, and Max(n) to support the sidekiq bool|int typing. The public builder interfaces for opts have now been moved to Into<RetryOpts> which will continue to work for true/false like before but also a numeric value. Also... I found that I hadn't impl'd the RetryMiddleware to respect a retry: false option, so that's been fixed. I also added max_retries to be plucked from the job if set and will be used in favor of whatever the value was for the worker since they can be different. TODO: Need to add a test for the new retry middleware logic. That shouldn't have been a bug in the first place.
|
@film42 I think it looks good, certainly better than a plain JsonValue! |
|
Sweet. I need to do a few more small things before cutting a new release. |
|
Thanks for getting this one started, @Antti ! ❤️ ❤️ ❤️ |
According to the sidekiq docs,
sidekiq_optionsretryparam can be either boolean or a number.Change the param of the retry argument to be a JsonValue for now, as it's a convenient wrapper over different basic types