-
Notifications
You must be signed in to change notification settings - Fork 107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Gush::Client
now delegates the ActiveJob.perform_later
call to Gush::Job
#117
Conversation
…ush::Job` This delegation allows for Gush users to easily override the enqueing behavior to their custom job class, e.g. to support additional ActiveJob options or enqueing behaviors (e.g synchronous with `perform_now`).
@@ -59,6 +59,14 @@ def enqueue! | |||
@failed_at = nil | |||
end | |||
|
|||
def enqueue_worker!(options = {}) | |||
Gush::Worker.set(options).perform_later(workflow_id, name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reads great! Simple and clean abstraction. It feels like no matter what else happens around it this should be the goal for the interface to the Worker class. Kudos!
Gush::Worker.set(queue: queue).perform_later(*[workflow_id, job.name]) | ||
end | ||
|
||
options = { queue: configuration.namespace }.merge(job.worker_options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overriding method approach definitely comes together well. It makes for a concise and flexible interface to the Job class. If I put on my horn-rimmed-dark-tinted critic spectacles (and squint pensively) it does read a little odd to have to reach into the #worker_options
method to merge back in to the #enqueue_worker!
method on the same worker object.
This is partly semantic, and you could clear it up maybe with a #default_worker_options
intermediary, or something similar, but I am not sure it is worth the additional layer without actually trying it.
To get anywhere with another approach would probably have to sacrifice some of the simplicity of this approach, so if the maintainers like it then so do I!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review and comments!
I originally considered this alternative:
class Gush::Client
def enqueue_job(workflow_id, job)
...
job.enqueue_worker!(queue: configuration.namespace)
end
end
class Gush::Job
def enqueue_worker!(default_options = {})
options = default_options.merge(worker_options)
Gush::Worker.set(options).perform_later(workflow_id, name)
end
end
but I think it's preferable to have Job#enqueue_worker!
receive everything it needs without having to do additional work to get options, and allow the extra line of complexity to be in the Client
class, since that's not meant to be overridden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you already tried the default semantic and have good reason to avoid it, thanks for the reply!
This delegation allows for Gush users to easily override the enqueing behavior to their custom job class, e.g. to support additional ActiveJob options or enqueing behaviors (e.g synchronous with
perform_now
).