From a13f18affcb8569cebea257b7ca38699950867a4 Mon Sep 17 00:00:00 2001 From: Jared Roesch Date: Tue, 15 Jul 2014 09:41:13 -0700 Subject: [PATCH 1/9] Initial refactor of attr_default to support copy --- lib/attr_default.rb | 47 ++++++++++++++++++++++++++------------- test/attr_default_test.rb | 27 +++++++++------------- 2 files changed, 42 insertions(+), 32 deletions(-) diff --git a/lib/attr_default.rb b/lib/attr_default.rb index 289eac1..97cdd05 100644 --- a/lib/attr_default.rb +++ b/lib/attr_default.rb @@ -88,10 +88,19 @@ def write_attribute_with_fixups(attr_name, args) def needs_time_zone_fixup?(attr_name) self.class.send(:create_time_zone_conversion_attribute?, attr_name, self.class.columns_hash[attr_name]) end - - if Gem.loaded_specs['activesupport'].version >= Gem::Version.new('3.1') - def dup - result = super + + def copy(opts = {}) + if opts.key? :new_record + result = + if defined?(super) + super + else + if opts[:new_record] + self.copy_new_record_true + else + self.copy_new_record_false # self.dup # what kind of default logic do we wire in + end + end result.created_at = nil unless !result.class.columns_hash.has_key?('created_at') result.updated_at = nil unless !result.class.columns_hash.has_key?('updated_at') if self.new_record? @@ -100,19 +109,27 @@ def dup result.instance_variable_set(:@_attr_defaults_set_from_dup, true) end result + else + # eventually phase this out with required keywords in ruby 2.0 + raise ArgumentError, "Ambiguous call to copy please provide :new_record => (true|false)" + end + end + + if RUBY_VERSION == '1.8.7' + def copy_new_record_true + clone + end + + def copy_new_record_false + dup end - alias_method(:clone, :dup) else - def clone - result = super - result.created_at = nil unless !result.class.columns_hash.has_key?('created_at') - result.updated_at = nil unless !result.class.columns_hash.has_key?('updated_at') - if self.new_record? - result.instance_variable_set(:@_attr_default_set, self._attr_default_set.dup) - else - result.instance_variable_set(:@_attr_defaults_set_from_dup, true) - end - result + def copy_new_record_true + dup + end + + def copy_new_record_false + clone end end end diff --git a/test/attr_default_test.rb b/test/attr_default_test.rb index 890b7a1..348533f 100644 --- a/test/attr_default_test.rb +++ b/test/attr_default_test.rb @@ -24,14 +24,7 @@ false end -DUP_METHODS = - if Gem.loaded_specs['activesupport'].version >= Gem::Version.new('4.0') - [:dup] - elsif Gem.loaded_specs['activesupport'].version >= Gem::Version.new('3.1') - [:dup, :clone] - else - [:clone] - end +COPY_METHODS = [:copy] File.unlink('test.sqlite3') rescue nil ActiveRecord::Base.logger = Logger.new(STDERR) @@ -220,27 +213,27 @@ def test_use_default_when_saved_if_not_touched assert_equal "initial.com", domain.domain end - def test_dup_and_clone_touched_state_when_duped_or_cloned_before_save_new_record_true - DUP_METHODS.each do |dup| + def test_copy_touched_state_when_copied_before_save_new_record_true + COPY_METHODS.each do |copy| u = TestUser.new :first_name => 'John', :last_name => 'Doe' u.last_name = 'overridden' - u2 = u.send(dup) + u2 = u.send(copy, :new_record => true) assert_equal 'overridden', u2.read_attribute(:last_name) assert_equal 'overridden', u2.last_name end end - def test_dup_and_clone_touched_state_when_duped_or_cloned_after_save_new_record_false - DUP_METHODS.each do |dup| + def test_copy_touched_state_when_copied_after_save_new_record_false + COPY_METHODS.each do |copy| u = TestUser.new(:first_name => 'John', :last_name => 'Doe') u.last_name = 'overridden' - u2 = u.send(dup) + u2 = u.send(copy, :new_record => false) u2.save! u.save! - assert u.send(dup).instance_variable_get(:@_attr_defaults_set_from_dup) - assert_equal 'overridden', u.send(dup).last_name + assert u.send(copy, :new_record => false).instance_variable_get(:@_attr_defaults_set_from_dup) + assert_equal 'overridden', u.send(copy, :new_record => false).last_name ufind = TestUser.find(u.id) - u3 = ufind.send(dup) + u3 = ufind.send(copy, :new_record => false) assert_equal 'overridden', u3.read_attribute(:last_name), u3.attributes.inspect assert_equal 'overridden', u3.last_name u3.save! From e3db97d220dc2a9b7ad07502ffd644e6b7605839 Mon Sep 17 00:00:00 2001 From: Jared Roesch Date: Tue, 15 Jul 2014 10:07:22 -0700 Subject: [PATCH 2/9] Fix versioning based on Rails not Ruby --- lib/attr_default.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/attr_default.rb b/lib/attr_default.rb index 97cdd05..cdea5c7 100644 --- a/lib/attr_default.rb +++ b/lib/attr_default.rb @@ -115,7 +115,7 @@ def copy(opts = {}) end end - if RUBY_VERSION == '1.8.7' + if Gem.loaded_specs['activesupport'].version >= Gem::Version.new('3.1') def copy_new_record_true clone end From d011a8ef1e3ccf50692883d65f3b2d8cc5b61e05 Mon Sep 17 00:00:00 2001 From: Jared Roesch Date: Tue, 15 Jul 2014 10:45:09 -0700 Subject: [PATCH 3/9] had flipped the conditional by accident --- lib/attr_default.rb | 8 ++++---- test/attr_default_test.rb | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/attr_default.rb b/lib/attr_default.rb index cdea5c7..7e71e6d 100644 --- a/lib/attr_default.rb +++ b/lib/attr_default.rb @@ -117,19 +117,19 @@ def copy(opts = {}) if Gem.loaded_specs['activesupport'].version >= Gem::Version.new('3.1') def copy_new_record_true - clone + dup end def copy_new_record_false - dup + clone end else def copy_new_record_true - dup + clone end def copy_new_record_false - clone + dup end end end diff --git a/test/attr_default_test.rb b/test/attr_default_test.rb index 348533f..faa429f 100644 --- a/test/attr_default_test.rb +++ b/test/attr_default_test.rb @@ -227,13 +227,13 @@ def test_copy_touched_state_when_copied_after_save_new_record_false COPY_METHODS.each do |copy| u = TestUser.new(:first_name => 'John', :last_name => 'Doe') u.last_name = 'overridden' - u2 = u.send(copy, :new_record => false) + u2 = u.send(copy, :new_record => true) u2.save! u.save! - assert u.send(copy, :new_record => false).instance_variable_get(:@_attr_defaults_set_from_dup) + assert u.send(copy, :new_record => true).instance_variable_get(:@_attr_defaults_set_from_dup) assert_equal 'overridden', u.send(copy, :new_record => false).last_name ufind = TestUser.find(u.id) - u3 = ufind.send(copy, :new_record => false) + u3 = ufind.send(copy, :new_record => true) assert_equal 'overridden', u3.read_attribute(:last_name), u3.attributes.inspect assert_equal 'overridden', u3.last_name u3.save! From 76504313f44ee29bf3969d26feeb783a824108cd Mon Sep 17 00:00:00 2001 From: Jared Roesch Date: Wed, 16 Jul 2014 09:22:19 -0700 Subject: [PATCH 4/9] properly pass arguments to the super invocation \b --- lib/attr_default.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/attr_default.rb b/lib/attr_default.rb index 7e71e6d..7170965 100644 --- a/lib/attr_default.rb +++ b/lib/attr_default.rb @@ -93,7 +93,7 @@ def copy(opts = {}) if opts.key? :new_record result = if defined?(super) - super + super(opts) else if opts[:new_record] self.copy_new_record_true From 53c4b68d391e2c5d7436ac7dd5c35b90c71290a1 Mon Sep 17 00:00:00 2001 From: Jared Roesch Date: Wed, 16 Jul 2014 14:29:13 -0700 Subject: [PATCH 5/9] fix misconception about how attr_default clone had worked before --- lib/attr_default.rb | 27 ++++++++++++++------------- test/attr_default_test.rb | 3 ++- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/lib/attr_default.rb b/lib/attr_default.rb index 7170965..e976eb0 100644 --- a/lib/attr_default.rb +++ b/lib/attr_default.rb @@ -91,24 +91,25 @@ def needs_time_zone_fixup?(attr_name) def copy(opts = {}) if opts.key? :new_record - result = - if defined?(super) + if opts[:new_record] + result = if defined?(super) super(opts) else - if opts[:new_record] - self.copy_new_record_true - else - self.copy_new_record_false # self.dup # what kind of default logic do we wire in - end + copy_new_record_true end - result.created_at = nil unless !result.class.columns_hash.has_key?('created_at') - result.updated_at = nil unless !result.class.columns_hash.has_key?('updated_at') - if self.new_record? - result.instance_variable_set(:@_attr_default_set, self._attr_default_set.dup) + + result.created_at = nil unless !result.class.columns_hash.has_key?('created_at') + result.updated_at = nil unless !result.class.columns_hash.has_key?('updated_at') + if self.new_record? + result.instance_variable_set(:@_attr_default_set, self._attr_default_set.dup) + else + result.instance_variable_set(:@_attr_defaults_set_from_dup, true) + end + + result else - result.instance_variable_set(:@_attr_defaults_set_from_dup, true) + self.copy_new_record_false # self.dup # what kind of default logic do we wire in end - result else # eventually phase this out with required keywords in ruby 2.0 raise ArgumentError, "Ambiguous call to copy please provide :new_record => (true|false)" diff --git a/test/attr_default_test.rb b/test/attr_default_test.rb index faa429f..0157d78 100644 --- a/test/attr_default_test.rb +++ b/test/attr_default_test.rb @@ -223,6 +223,7 @@ def test_copy_touched_state_when_copied_before_save_new_record_true end end + # This test is referring to the record itself returning false from #new_record? after being "cloned" i.e copy(new_record: true) def test_copy_touched_state_when_copied_after_save_new_record_false COPY_METHODS.each do |copy| u = TestUser.new(:first_name => 'John', :last_name => 'Doe') @@ -231,7 +232,7 @@ def test_copy_touched_state_when_copied_after_save_new_record_false u2.save! u.save! assert u.send(copy, :new_record => true).instance_variable_get(:@_attr_defaults_set_from_dup) - assert_equal 'overridden', u.send(copy, :new_record => false).last_name + assert_equal 'overridden', u.send(copy, :new_record => true).last_name ufind = TestUser.find(u.id) u3 = ufind.send(copy, :new_record => true) assert_equal 'overridden', u3.read_attribute(:last_name), u3.attributes.inspect From 4366cc0b778522805a7b7ba0f00fa6ff22ec00ef Mon Sep 17 00:00:00 2001 From: Jared Roesch Date: Wed, 16 Jul 2014 14:45:53 -0700 Subject: [PATCH 6/9] fix issue with super dispatch --- lib/attr_default.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/attr_default.rb b/lib/attr_default.rb index e976eb0..d8d8e9a 100644 --- a/lib/attr_default.rb +++ b/lib/attr_default.rb @@ -108,7 +108,11 @@ def copy(opts = {}) result else - self.copy_new_record_false # self.dup # what kind of default logic do we wire in + if defined?(super) + super(opts) + else + copy_new_record_false # self.dup # what kind of default logic do we wire in + end end else # eventually phase this out with required keywords in ruby 2.0 From 4bbb87dbdf1d0db938147bf94899695ad7ff7c6d Mon Sep 17 00:00:00 2001 From: Jared Roesch Date: Fri, 18 Jul 2014 09:09:23 -0700 Subject: [PATCH 7/9] modify attr_default to perform the behavior that was actually expected before --- lib/attr_default.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/attr_default.rb b/lib/attr_default.rb index d8d8e9a..6159584 100644 --- a/lib/attr_default.rb +++ b/lib/attr_default.rb @@ -111,7 +111,7 @@ def copy(opts = {}) if defined?(super) super(opts) else - copy_new_record_false # self.dup # what kind of default logic do we wire in + copy_new_record_true # self.dup # what kind of default logic do we wire in end end else From 14838587b56c710b0d458787d91cdeb6e680440c Mon Sep 17 00:00:00 2001 From: Jared Roesch Date: Fri, 18 Jul 2014 14:04:10 -0700 Subject: [PATCH 8/9] clean up method aliasing --- lib/attr_default.rb | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/lib/attr_default.rb b/lib/attr_default.rb index 6159584..8483113 100644 --- a/lib/attr_default.rb +++ b/lib/attr_default.rb @@ -89,6 +89,7 @@ def needs_time_zone_fixup?(attr_name) self.class.send(:create_time_zone_conversion_attribute?, attr_name, self.class.columns_hash[attr_name]) end + # Talk to Colin about this def copy(opts = {}) if opts.key? :new_record if opts[:new_record] @@ -121,21 +122,11 @@ def copy(opts = {}) end if Gem.loaded_specs['activesupport'].version >= Gem::Version.new('3.1') - def copy_new_record_true - dup - end - - def copy_new_record_false - clone - end + alias :copy_new_record_true :dup + alias :copy_new_record_false :clone else - def copy_new_record_true - clone - end - - def copy_new_record_false - dup - end + alias :copy_new_record_true :clone + alias :copy_new_record_false :dup end end end From 958abe82fe05ab8c3eaba14c21ac1649956ae178 Mon Sep 17 00:00:00 2001 From: Jared Roesch Date: Mon, 21 Jul 2014 00:54:32 -0700 Subject: [PATCH 9/9] Fix issue in reasoning about situtation pre Ruby 2 --- lib/attr_default.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/attr_default.rb b/lib/attr_default.rb index 8483113..7b64039 100644 --- a/lib/attr_default.rb +++ b/lib/attr_default.rb @@ -89,7 +89,6 @@ def needs_time_zone_fixup?(attr_name) self.class.send(:create_time_zone_conversion_attribute?, attr_name, self.class.columns_hash[attr_name]) end - # Talk to Colin about this def copy(opts = {}) if opts.key? :new_record if opts[:new_record] @@ -112,7 +111,7 @@ def copy(opts = {}) if defined?(super) super(opts) else - copy_new_record_true # self.dup # what kind of default logic do we wire in + copy_new_record_false # self.dup # what kind of default logic do we wire in end end else