-
Notifications
You must be signed in to change notification settings - Fork 25
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
Allow memoization when include
d modules prepend MemoWise
and define initialize
#327
base: main
Are you sure you want to change the base?
Conversation
adbfd13
to
21040dc
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #327 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 190 196 +6
Branches 90 90
=========================================
+ Hits 190 196 +6 ☔ View full report in Codecov by Sentry. |
21040dc
to
55e1ec8
Compare
…ne `initialize` Fixes #302 Co-authored-by: alpaca-tc <[email protected]>
55e1ec8
to
12ce065
Compare
include
d modules prepend MemoWise
and define initialize
@@ -94,6 +96,15 @@ def inherited(subclass) | |||
end | |||
private_constant(:CreateMemoWiseStateOnInherited) | |||
|
|||
module CreateMemoWiseStateOnIncluded | |||
def included(base) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alpaca-tc in your code example you had an additional conditional here and below. Am I missing the reasoning behind those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, I don't want to define unnecessary methods, so I added a branch.
BTW, I found a bug that memo_wise is not initialized when a module is included in a class with an included module.
We can fix this case by simply making the following change, but there may be other edge cases.
Something may need to be done to reduce the complexity.
diff --git a/spec/memo_wise_spec.rb b/spec/memo_wise_spec.rb
index 6e73d3a..a2355a1 100644
--- a/spec/memo_wise_spec.rb
+++ b/spec/memo_wise_spec.rb
@@ -365,18 +365,32 @@ RSpec.describe MemoWise do
end
end
+ let(:nested_module_with_initializer) do
+ Module.new do
+ include Module3
+ def initialize(*); end
+ end
+ end
+
let(:klass_with_module_with_initializer) do
Class.new do
include Module3
end
end
+ let(:klass_with_nested_module_with_initializer) do
+ Class.new do
+ include Module4
+ end
+ end
+
let(:instance) { klass.new }
before(:each) do
stub_const("Module1", module1)
stub_const("Module2", module2)
stub_const("Module3", module_with_initializer)
+ stub_const("Module4", nested_module_with_initializer)
end
it "memoizes inherited methods separately" do
@@ -402,6 +416,14 @@ RSpec.describe MemoWise do
expect(instance.module1_method_counter).to eq(1)
end
+ it "can memoize klass with nested module with initializer" do
+ instance = klass_with_nested_module_with_initializer.new(true)
+ expect { instance.module1_method }.not_to raise_error
+
+ expect(Array.new(4) { instance.module1_method }).to all eq("module1_method")
+ expect(instance.module1_method_counter).to eq(1)
+ end
+
it "can reset klass with initializer" do
instance = klass_with_initializer.new(true)
expect { instance.reset_memo_wise }.not_to raise_error
@@ -411,6 +433,11 @@ RSpec.describe MemoWise do
instance = klass_with_module_with_initializer.new(true)
expect { instance.reset_memo_wise }.not_to raise_error
end
+
+ it "can reset klass with nested module with initializer" do
+ instance = klass_with_nested_module_with_initializer.new(true)
+ expect { instance.reset_memo_wise }.not_to raise_error
+ end
end
context "when the class, its superclass, and its module all memoize methods" do
diff --git a/lib/memo_wise.rb b/lib/memo_wise.rb
index e793603..4a5ee2a 100644
--- a/lib/memo_wise.rb
+++ b/lib/memo_wise.rb
@@ -97,10 +97,16 @@ module MemoWise
private_constant(:CreateMemoWiseStateOnInherited)
module CreateMemoWiseStateOnIncluded
+ module CreateMemoWiseStateOnInitialize
+ class_eval(INITIALIZE_LITERAL, __FILE__, __LINE__ + 1)
+ end
+
def included(base)
- base.prepend(Module.new do
- class_eval(INITIALIZE_LITERAL, __FILE__, __LINE__ + 1)
- end)
+ if base.is_a?(Class)
+ base.prepend(CreateMemoWiseStateOnInitialize)
+ elsif !base.singleton_class?
+ base.singleton_class.prepend(CreateMemoWiseStateOnIncluded)
+ end
end
end
private_constant(:CreateMemoWiseStateOnIncluded)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #302
Co-authored-by: @alpaca-tc
Before merging:
README.md
and update this PRCHANGELOG.md
, add an entry following Keep a Changelog guidelines with semantic versioning