Closed gregory closed 2 years ago
What benefits do you have in mind when using this approach? On May 17, 2015 7:20 PM, "Grégory Horion" notifications@github.com wrote:
Hey,
This looks pretty cool. One improvement would be to include modules to decorate the classes instead of using inheritance. Here is what i have in mind:
workflows/sample_workflow.rbclass SampleWorkflow
include Gush::Workflow gush do run FetchJob1 run FetchJob2, params: {some_flag: true, url: 'http://url.com'}
run PersistJob1, after: FetchJob1 run PersistJob2, after: FetchJob2 run Normalize, after: [PersistJob1, PersistJob2], before: Index run Index
endend
class FetchJob1 include Gush::Job def self.perform(params={})
do some fetching from remote APIs
params #=> {url: "http://some.com/url"}
endend
what do you think?
— Reply to this email directly or view it on GitHub https://github.com/pokonski/gush/issues/6.
Using inheritance is a constraint that force me the have classes that are Gush::Workflow or Gush::Job and nothing else, where in practice, it's not always the case.
In the following example, i have my SignupWorkflow
that already inherit from a class. Of course, i do not want to make ClassA to inherit from Gush::Workflow cause it is not. In this simple case, how could i make SignupWorkflow a Gush::Workflow?
class ClassA
# some stuffs
end
class SignupWorkflow < ClassA; end
Same idea for the job, how should i use Gush here and make User::EmailResetPassword
a Gush::Job
? :)
class User::EmailResetPassword < SomeClass; end
Adding a specific DSL by including Gush::Workflow might not only allow me to use it with classes that already inherit from another one, but also prevent class method name conflicts.
Inheriting workflows makes zero sense, because jobs are defined on the instance level. But will change to include
s.
Also the block gush do
doesn't really add anything beside pointless complexity.
@gregory the only purpose for *Workflow
classes is to provide a DSL for workflows and they're never meant to be instantiated by the users. Look at the DSL that rack uses for configuration. Did you ever need to customize the way how rack apps are configured?
Using custom subclasses for Job
s makes more sense, but I can't see why would it be better than something like this:
class ResetEmailJob < Gush::Job
def work
User::EmailResetPassword.new.whatever(params)
end
end
@pokonski for the gush do
i actually meant to avoid using #configure
to avoid method name conflict with a class that would have that method already defined...
This doesnt makes sens to you?
class ClassA
def common_params
end
end
class Workflow1 < ClassA
def self.configure
run FetchJob1, params: common_params
end
end
class Workflow2 < ClassA
def self.configure
run FetchJob2, params: common_params
end
end
@mlen If this is a DSL, why should that be nested in a configure method? The DSL should be enough right?
class ResetEmailJob < Gush::Job
def work
User::EmailResetPassword.new.whatever(params)
end
end
=> Now you are forcing me into your architecture style. That would mean that in order to have a job, i'd have to create new classes in my codebase just to wrap the call? ...
To be honnest, i'm not even sure why we need to include/inherit from Gush::Job we should just call .perform just like Resque does, and maybe open to customization like this: run FetchJob2, params: {some_flag: true, url: 'http://url.com'}, perform_method: :foobar
what do you guys think about that?
In version 1.0 the jobs will no longer need to be inherited from Gush::Job, so stay tuned!
Closing due to inactivity and general change of direction since my comment
Hey,
This looks pretty cool. One improvement would be to include modules to decorate the classes instead of using inheritance. Here is what i have in mind:
what do you think?