Skip to content

Add inheritance for hooks#138

Closed
vsh91 wants to merge 2 commits into
collectiveidea:masterfrom
vsh91:hooks-inheritance
Closed

Add inheritance for hooks#138
vsh91 wants to merge 2 commits into
collectiveidea:masterfrom
vsh91:hooks-inheritance

Conversation

@vsh91

@vsh91 vsh91 commented Apr 22, 2017

Copy link
Copy Markdown

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

m20io commented May 11, 2018

Copy link
Copy Markdown

👍

@fluke

fluke commented Jun 1, 2018

Copy link
Copy Markdown

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

@laserlemon

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

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

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

povilasjurcys commented Jul 24, 2019

Copy link
Copy Markdown

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

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

@matiasgarcia

Copy link
Copy Markdown

@laserlemon please merge this :)

@badBlackShark

Copy link
Copy Markdown

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

vsh91 commented Feb 8, 2023

Copy link
Copy Markdown
Author

#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