From 4ce196b7cdc760f787f04cd5f372f5e6adefaaef Mon Sep 17 00:00:00 2001 From: Ashu Pachauri Date: Thu, 12 Jul 2018 13:43:19 +0530 Subject: [PATCH 1/3] Fix for optional arguments Use a fixed :memoist_reload argument instead of depending on argument order as it is fragile in case of optional and named args. This change maintains backward compatibility for methods that don't have optional/named arguments. --- lib/memoist.rb | 9 +++++++++ test/memoist_test.rb | 12 +++++++----- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/lib/memoist.rb b/lib/memoist.rb index 54b67e6..fb99716 100644 --- a/lib/memoist.rb +++ b/lib/memoist.rb @@ -59,6 +59,15 @@ def self.memoist_eval(klass, *args, &block) end def self.extract_reload!(method, args) + # Arity is negative for methods with optional/named args + # We don't want to extract out the last argument in this case. + # Also, if the :memoist_reload flag is passed, we use that + # irrespective of the arity. + if method.arity.negative? || args.include?(:memoist_reload) + reload = args.delete :memoist_reload + return !reload.nil? + end + if args.length == method.arity.abs + 1 && (args.last == true || args.last == :reload) reload = args.pop end diff --git a/test/memoist_test.rb b/test/memoist_test.rb index 49a09d0..68f0e17 100644 --- a/test/memoist_test.rb +++ b/test/memoist_test.rb @@ -67,7 +67,7 @@ def age memoize :name, :age - def sleep(hours = 8) + def sleep(initial_delay, hours = 8, mins = 5) @counter.call(:sleep) hours end @@ -244,13 +244,15 @@ def test_memoization end def test_memoize_with_optional_arguments - assert_equal 4, @person.sleep(4) + assert_equal 4, @person.sleep(4, 4) assert_equal 1, @person.sleep_calls - 3.times { assert_equal 4, @person.sleep(4) } + 3.times { assert_equal 4, @person.sleep(4, 4) } assert_equal 1, @person.sleep_calls - 3.times { assert_equal 4, @person.sleep(4, :reload) } + # There has been exactly one sleep call due to memoization + # Now, with reload, we'll have 3 more calls + 3.times { assert_equal 10, @person.sleep(4, 10,:memoist_reload) } assert_equal 4, @person.sleep_calls end @@ -261,7 +263,7 @@ def test_memoize_with_options_hash 3.times { assert_equal true, @person.update_attributes(age: 21, name: 'James') } assert_equal 1, @person.update_attributes_calls - 3.times { assert_equal true, @person.update_attributes({ age: 21, name: 'James' }, :reload) } + 3.times { assert_equal true, @person.update_attributes({ age: 21, name: 'James' }, :memoist_reload) } assert_equal 4, @person.update_attributes_calls end From 45fb04ac8ca9a3a506697f37eecf7867467aca77 Mon Sep 17 00:00:00 2001 From: Ashu Pachauri Date: Thu, 12 Jul 2018 14:22:58 +0530 Subject: [PATCH 2/3] Use named argument for memoist_reload --- lib/memoist.rb | 13 ++++++++++--- test/memoist_test.rb | 4 ++-- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/lib/memoist.rb b/lib/memoist.rb index fb99716..81d9ba4 100644 --- a/lib/memoist.rb +++ b/lib/memoist.rb @@ -63,9 +63,16 @@ def self.extract_reload!(method, args) # We don't want to extract out the last argument in this case. # Also, if the :memoist_reload flag is passed, we use that # irrespective of the arity. - if method.arity.negative? || args.include?(:memoist_reload) - reload = args.delete :memoist_reload - return !reload.nil? + if method.arity.negative? && args.length > 0 && args.last.is_a?(Hash) + named_args_hash = args.last + reload = named_args_hash.delete :memoist_reload + # If no other named args were passed, that means memoist_reload is an + # extra arg that we need to get rid of; otherwise Ruby will complain + # about argument number mismatch. + if named_args_hash.empty? + args.delete(named_args_hash) + end + return reload end if args.length == method.arity.abs + 1 && (args.last == true || args.last == :reload) diff --git a/test/memoist_test.rb b/test/memoist_test.rb index 68f0e17..623d3bc 100644 --- a/test/memoist_test.rb +++ b/test/memoist_test.rb @@ -252,7 +252,7 @@ def test_memoize_with_optional_arguments # There has been exactly one sleep call due to memoization # Now, with reload, we'll have 3 more calls - 3.times { assert_equal 10, @person.sleep(4, 10,:memoist_reload) } + 3.times { assert_equal 10, @person.sleep(4, 10, memoist_reload: true) } assert_equal 4, @person.sleep_calls end @@ -263,7 +263,7 @@ def test_memoize_with_options_hash 3.times { assert_equal true, @person.update_attributes(age: 21, name: 'James') } assert_equal 1, @person.update_attributes_calls - 3.times { assert_equal true, @person.update_attributes({ age: 21, name: 'James' }, :memoist_reload) } + 3.times { assert_equal true, @person.update_attributes({ age: 21, name: 'James' }, memoist_reload: true) } assert_equal 4, @person.update_attributes_calls end From 5c21f4720b494cc4ea7676455b33d77972deacaf Mon Sep 17 00:00:00 2001 From: Ashu Pachauri Date: Thu, 12 Jul 2018 14:58:32 +0530 Subject: [PATCH 3/3] Use memoist_reload flag for all methods --- lib/memoist.rb | 6 ++++-- test/memoist_test.rb | 20 ++++++++++++++------ 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/lib/memoist.rb b/lib/memoist.rb index 81d9ba4..256b1bf 100644 --- a/lib/memoist.rb +++ b/lib/memoist.rb @@ -63,7 +63,7 @@ def self.extract_reload!(method, args) # We don't want to extract out the last argument in this case. # Also, if the :memoist_reload flag is passed, we use that # irrespective of the arity. - if method.arity.negative? && args.length > 0 && args.last.is_a?(Hash) + if args.length > 0 && args.last.is_a?(Hash) named_args_hash = args.last reload = named_args_hash.delete :memoist_reload # If no other named args were passed, that means memoist_reload is an @@ -74,8 +74,10 @@ def self.extract_reload!(method, args) end return reload end + return false if method.arity.negative? - if args.length == method.arity.abs + 1 && (args.last == true || args.last == :reload) + # Legacy purposes, works only for methods with no optional/named args + if args.length == method.arity + 1 && (args.last == true || args.last == :reload) reload = args.pop end reload diff --git a/test/memoist_test.rb b/test/memoist_test.rb index 623d3bc..f14f2bc 100644 --- a/test/memoist_test.rb +++ b/test/memoist_test.rb @@ -67,9 +67,9 @@ def age memoize :name, :age - def sleep(initial_delay, hours = 8, mins = 5) + def sleep(hours, mins = 5, seconds: 0) @counter.call(:sleep) - hours + hours * 3600 + mins * 60 + seconds end memoize :sleep @@ -241,19 +241,25 @@ def test_memoization 3.times { assert_equal 'Josh', @person.name } assert_equal 1, @person.name_calls + + 3.times { assert_equal 'Josh', @person.name(memoist_reload: true) } + assert_equal 4, @person.name_calls end def test_memoize_with_optional_arguments - assert_equal 4, @person.sleep(4, 4) + assert_equal 15000, @person.sleep(4, 10) assert_equal 1, @person.sleep_calls - 3.times { assert_equal 4, @person.sleep(4, 4) } + 3.times { assert_equal 15000, @person.sleep(4, 10) } assert_equal 1, @person.sleep_calls + 3.times { assert_equal 15000, @person.sleep(4, 10, seconds: 0) } + assert_equal 2, @person.sleep_calls + # There has been exactly one sleep call due to memoization # Now, with reload, we'll have 3 more calls - 3.times { assert_equal 10, @person.sleep(4, 10, memoist_reload: true) } - assert_equal 4, @person.sleep_calls + 3.times { assert_equal 15000, @person.sleep(4, 10, memoist_reload: true) } + assert_equal 5, @person.sleep_calls end def test_memoize_with_options_hash @@ -295,6 +301,8 @@ def test_reloadable assert_equal 2, @calculator.counter assert_equal 3, @calculator.counter(true) assert_equal 3, @calculator.counter + assert_equal 4, @calculator.counter(memoist_reload: true) + assert_equal 4, @calculator.counter end def test_flush_cache