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

Use prepend instead of alias_method_chain like approach #10

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

atd
Copy link
Contributor

@atd atd commented Nov 7, 2024

The current implementation is using an old approach of renaming decorated methods. This approach has some drawbacks, like the inability to call super, confusing method names, and other issues explained here by Justin Weiss
https://www.justinweiss.com/articles/rails-5-module-number-prepend-and-the-end-of-alias-method-chain/

An alternative was proposed in Rails 5: using Ruby's prepend module to define the methods that need to be decorated.

This commit creates a new ClassName::CaptainHookDecorator module in each class where hook is called, prepends it to the class, and defines the relevant decorator methods in that module

Current implementation is using an old approach of renaming
decorated methods. This approach has some drawback, like the inhability
to call super, confusing method names, and other issues explanined here
by Justin Weiss
https://www.justinweiss.com/articles/rails-5-module-number-prepend-and-the-end-of-alias-method-chain/

The solution was proposed in Rails 5: using Ruby's prepend module to
define the methods that need to be decorated.

This commit creates a new `ClassName::CaptainHookDecorator` module in
each class where `hook` is called, prepends it to the class, and defines
the relevant decorator methods in that module
Copy link
Member

@gtrias gtrias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow!! This is amazing 👏

Have you tried with factorial?

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.

2 participants