Make Module#prepend take a block #1639
Conversation
adelcambre
force-pushed
the
acd/prepend-block
branch
from
76867f5
to
93a5aa0
Compare
|
You can write the former without OriginalClass.prepend Module.new {
def some_original_method
do_something_else
super
end
} |
eval.c
Outdated
|
|
||
| CONST_ID(id_prepend_features, "prepend_features"); | ||
| CONST_ID(id_prepended, "prepended"); | ||
|
|
||
| rb_check_arity(argc, 1, UNLIMITED_ARGUMENTS); | ||
| if(rb_block_given_p()) { |
There was a problem hiding this comment.
should check arity when a block is given.
There was a problem hiding this comment.
@nobu My assumption in this case is that if a block is given then no arguments are required, which would then result in an arity check of 0, UNLIMITED_ARGUMENTS which as I understand it would be a noop. Is that not the case?
There was a problem hiding this comment.
To allow any number of arguments and to ignore given arguments silently are different.
There was a problem hiding this comment.
The PR as it currently stands still uses the other modules passed on the command line too. It doesn't ignore any arguments.
There was a problem hiding this comment.
I added another test showing this behavior in 3331f39
eval.c
Outdated
|
|
||
| CONST_ID(id_prepend_features, "prepend_features"); | ||
| CONST_ID(id_prepended, "prepended"); | ||
|
|
||
| rb_check_arity(argc, 1, UNLIMITED_ARGUMENTS); | ||
| if(rb_block_given_p()) { | ||
| new_mod = rb_module_new(); |
There was a problem hiding this comment.
adjust the style and indent.
in general, not only ruby, you should respect the style in the original files, not your favorite style.
There was a problem hiding this comment.
I attempted to more closely match the style in 0439e5c. There were already two indentation styles so I tried to follow the one that seemed more common.
test/ruby/test_module.rb
Outdated
| @@ -1572,6 1572,16 @@ def test_prepend | |||
| assert_equal(expected, obj.m1) | |||
| end | |||
|
|
|||
| def test_prepend_block | |||
| C0.prepend do | |||
There was a problem hiding this comment.
Isn't this modifying C0 for all following tests? If you'll run test_prepend after test_prepend_block, will it fail?
There was a problem hiding this comment.
@simi I couldn't get it to fail in any order, but that's a great point. I modified the test to just make a dynamic class in the test rather than modifying C0.
In our codebase we have a few instances where we need, for a variety of reasons, to monkey patch a class. With the advent of
Module#prepend, this is now much cleaner and safer than the days ofalias_method_chainfrom Rails or carefully aliasing and calling the original in plain ruby.We have a pattern when we do these kinds of things that look like this:
This works great, but it's not the nicest syntax. It would be really nice if you could just pass a block directly to
Module#prependthat's what this PR does. This would allow one to rewrite the snippet above more like this: