chore(invokers): Refactor callback invokers, add class-callbacks support #541
Conversation
|
Hi @pandomic . Thank you for raising this PR. I will go through changes over the coming weekend. |
|
@alto Need your review on this. Can you have a look once ? |
|
@anilmaurya Sure. |
aasm.gemspec
Outdated
| @@ -23,6 +23,7 @@ Gem::Specification.new do |s| | |||
| s.add_development_dependency 'rspec', ">= 3" | |||
| s.add_development_dependency 'generator_spec' | |||
| s.add_development_dependency 'appraisal' | |||
| s.add_development_dependency 'rubocop' | |||
There was a problem hiding this comment.
Please not. If you really think we benefit from rubocop, feel free to open a pull request just for that. But it's off-topic for this one.
| @@ -13,7 +15,13 @@ def initialize(name, klass, state_machine, options={}) | |||
| def initialize_copy(orig) | |||
| super | |||
| @options = {} | |||
| orig.options.each_pair { |name, setting| @options[name] = setting.is_a?(Hash) || setting.is_a?(Array) ? setting.dup : setting } | |||
| orig.options.each_pair do |name, setting| | |||
There was a problem hiding this comment.
Reformatted to make easier to read 👍
|
Thank you guys, will try to make some updates during the weekend |
pandomic
force-pushed
the
chores/refactor-callback-invokers
branch
from
dfb968b to
e43b874
Compare
|
Hey @alto, please take a look one more time when you will have some time ;) |
|
@pandomic Looks good now. Cheers! |
|
@pandomic Can you please resolve conflicts ? |
|
Hi @pandomic , Thank you for your contribution. |
|
Hi @anilmaurya, thank you a lot! Sorry, had no time to resolve conflicts, hope there was not too many |
|
Nice change 👍 Just a quick question: |
|
oh, yeah, looks like the first one is not needed anymore, as the second one explains behavior a bit more in detail |
|
@tisba Thanks for pointing it out. 👍 |
Motivation
While working on a big project we faced with a problem - our models started to grow, we needed to make a huge processing on state/transition/event callbacks, and keeping that logic in procs or in a model methods was not a good solution for us. Currently we are using a monkey-patched version of invoking logic to add a class-callbacks support. But I guess there also might be people who faced the same problem and need this feature supported officially.
While diving into the AASM code we found that invoking logic is mostly duplicated, so we decided to extract duplicated parts and add a class-callbacks support.
Related issue: #503
A few words about PR
PR contains base invoking class and concrete invokers for each subject type (Proc, Literal, Class). Base invoker decides which concrete invoker to use and also takes some additional configuration to support states/events/transitions (as they may have some differences in default return values, options and etc.)
Breaking changes