Skip to content
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

Add inheritance for hooks #138

Closed
wants to merge 2 commits into from

Conversation

vsh91
Copy link

@vsh91 vsh91 commented Apr 22, 2017

Hey,

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

Best,
Viktor

babelian added a commit to pledgemusic/interactor that referenced this pull request Dec 1, 2017
@m20io
Copy link

m20io commented May 11, 2018

👍

@fluke
Copy link

fluke commented Jun 1, 2018

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

@laserlemon
Copy link
Collaborator

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
Copy link

@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
Copy link

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
Copy link

povilasjurcys commented Jul 24, 2019

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: vsh91#1 Can you provide any example where @vsh91 approach fails?

@povilasjurcys
Copy link

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

@matiasgarcia
Copy link

@laserlemon please merge this :)

@badBlackShark
Copy link

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
Copy link
Author

vsh91 commented Feb 8, 2023

#191

@vsh91 vsh91 closed this Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants