collectiveidea / interactor

Interactor provides a common interface for performing complex user interactions.
MIT License
3.36k stars 212 forks source link

Add inheritance for hooks #138

Closed vsh91 closed 1 year ago

vsh91 commented 7 years ago

Hey,

First off, thank you for gem. This pull request is going to fix the bug with hooks inheritance #114

Best, Viktor

m20io commented 6 years ago

👍

fluke commented 6 years ago

@laserlemon Could this be merged in. Would make the Interactor classes more intuitive.

laserlemon commented 6 years ago

Could you please try to add some tests that assert that hooks added to the child class are not also added to the parent? Also, the implementation is a little verbose. Isn't there some cool @@ class variable or cattr_accessor trick that's perfect for this sort of thing?

babelian commented 5 years ago

@laserlemon looks like @vsh91 added those specs, could this be merged (and version bumped) i'm using interactor in a gemspec and can only link to a rubygem (have been using github/ref in the Gemfile but that's no longer possible).

many thanks.

arpitchauhan commented 5 years ago

I think we should add hooks belonging to all ancestors (in proper order), not just those that belong to the (immediate) superclass. Ancestors also include the modules that a class has included, btw.

povilasjurcys commented 5 years ago

hey @arpitchauhan . Although I'm more fan of using Class#inherited approach:

    def inherited(child_class)
      child_class.instance_variable_set(:@before_hooks, before_hooks.dup)
      child_class.instance_variable_set(:@around_hooks, around_hooks.dup)
      child_class.instance_variable_set(:@after_hooks, after_hooks.dup)
    end

but I have to admit, that @vsh91 approach works too. I added extra tests for "deep inheritance" case and it seems that it works: https://github.com/vsh91/interactor/pull/1 Can you provide any example where @vsh91 approach fails?

povilasjurcys commented 5 years ago

@laserlemon this PR is opened for almost 2 years. Can we merge this feature?

matiasgarcia commented 5 years ago

@laserlemon please merge this :)

badBlackShark commented 1 year ago

What's the holdup in merging this? This has been open for over five and a half years and is currently causing issues for the project I'm working on.

vsh91 commented 1 year ago

https://github.com/collectiveidea/interactor/pull/191