From ad1b7f9e7c46134fbedb8c8818c6748accae2026 Mon Sep 17 00:00:00 2001 From: Jan van der Pas Date: Thu, 4 Sep 2014 00:23:21 +0200 Subject: [PATCH 1/3] Add the :allow_nil option, which is true by default, and its #allow_nil? predicate on the Virtus::Attribute class --- lib/virtus/attribute.rb | 20 ++++++++++++++++++- .../attribute/allow_nil_predicate_spec.rb | 19 ++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) create mode 100644 spec/unit/virtus/attribute/allow_nil_predicate_spec.rb diff --git a/lib/virtus/attribute.rb b/lib/virtus/attribute.rb index 369eadfb..2b4d34cc 100644 --- a/lib/virtus/attribute.rb +++ b/lib/virtus/attribute.rb @@ -20,12 +20,13 @@ class Attribute include ::Equalizer.new(:type, :options) - accept_options :primitive, :accessor, :default, :lazy, :strict, :required, :finalize + accept_options :primitive, :accessor, :default, :lazy, :strict, :required, :finalize, :allow_nil strict false required true accessor :public finalize true + allow_nil true # @see Virtus.coerce # @@ -210,6 +211,23 @@ def finalized? frozen? end + # Return if the attribute accepts nil as value when default value is set + # + # @example + # + # attr = Virtus::Attribute.build(String, :default => 'foo', :allow_nil => true) + # attr.allow_nil? # => true + # + # attr = Virtus::Attribute.build(String, :default => 'foo', :allow_nil => false) + # attr.allow_nil? # => false + # + # @return [Boolean] + # + # @api public + def allow_nil? + options[:allow_nil] + end + # @api private def define_accessor_methods(attribute_set) attribute_set.define_reader_method(self, name, options[:reader]) diff --git a/spec/unit/virtus/attribute/allow_nil_predicate_spec.rb b/spec/unit/virtus/attribute/allow_nil_predicate_spec.rb new file mode 100644 index 00000000..bd9691b5 --- /dev/null +++ b/spec/unit/virtus/attribute/allow_nil_predicate_spec.rb @@ -0,0 +1,19 @@ +require 'spec_helper' + +describe Virtus::Attribute, '#allow_nil?' do + subject { object.allow_nil? } + + let(:object) { described_class.build(String, :allow_nil => allow_nil) } + + context 'when allow_nil option is true' do + let(:allow_nil) { true } + + it { should be(true) } + end + + context 'when allow_nil option is false' do + let(:allow_nil) { false } + + it { should be(false) } + end +end From b0b941c3b31cfda776e975561897bf8df0ffabaa Mon Sep 17 00:00:00 2001 From: Jan van der Pas Date: Thu, 4 Sep 2014 00:28:08 +0200 Subject: [PATCH 2/3] Add the Virtus::Attribute::Accessor#present? predicate, to determine the presence of the attribute on the Virtus instance given the :allow_nil option --- lib/virtus/attribute/accessor.rb | 12 +++++++ spec/unit/virtus/attribute/present_spec.rb | 40 ++++++++++++++++++++++ 2 files changed, 52 insertions(+) create mode 100644 spec/unit/virtus/attribute/present_spec.rb diff --git a/lib/virtus/attribute/accessor.rb b/lib/virtus/attribute/accessor.rb index 7a51f03d..a10b722a 100644 --- a/lib/virtus/attribute/accessor.rb +++ b/lib/virtus/attribute/accessor.rb @@ -34,6 +34,18 @@ def self.extended(descendant) descendant.instance_variable_set('@instance_variable_name', "@#{name}") end + # Return if attribute value is something other than + # a nil value when attribute does not accept nil values + # + # @param [Object] instance + # + # @return [Boolean] + # + # @api public + def present?(instance) + self.allow_nil? ? self.defined?(instance) : !instance.instance_variable_get(instance_variable_name).nil? + end + # Return if attribute value is defined # # @param [Object] instance diff --git a/spec/unit/virtus/attribute/present_spec.rb b/spec/unit/virtus/attribute/present_spec.rb new file mode 100644 index 00000000..28167ff5 --- /dev/null +++ b/spec/unit/virtus/attribute/present_spec.rb @@ -0,0 +1,40 @@ +require 'spec_helper' + +describe Virtus::Attribute, '#present?' do + subject { object.present?(instance) } + + let(:model) { Class.new { attr_accessor :test } } + let(:name) { :test } + let(:instance) { model.new } + + context 'when the attribute allows nil values' do + let(:object) { described_class.build(String, :name => name, :allow_nil => true) } + + context 'and the attribute value is defined as nil' do + before { instance.test = nil } + it { should be(true) } + end + + context 'and the attribute value is NOT defined' do + it { should be(false) } + end + end + + context 'when the attribute does NOT allow nil values' do + let(:object) { described_class.build(String, :name => name, :allow_nil => false) } + + context 'and the attribute value is defined' do + before { instance.test = 'foo' } + it { should be(true) } + end + + context 'and the attribute value is defined as nil' do + before { instance.test = nil } + it { should be(false) } + end + + context 'and the attribute value is NOT defined' do + it { should be(false) } + end + end +end From a75ba02095e92151b558f50f72ccc1868318635d Mon Sep 17 00:00:00 2001 From: Jan van der Pas Date: Thu, 4 Sep 2014 00:45:06 +0200 Subject: [PATCH 3/3] Implement the feature of allowing a default value when the attribute is assigned as nil: - Replace the Virtus::Attribute::Accessor#defined? method for the #present? method in the Virtus::AttributeSet#skip_default? private method - Replace the #instance_variable_defined? method for the Virtus::Attribute::Accessor#present? method in the Virtus::Attribute::LazyDefault#get method - Update the README with the documentation of this feature This fixes Github issue #291 (https://github.com/solnic/virtus/issues/291) and #172 (https://github.com/solnic/virtus/issues/172) --- README.md | 7 ++++++- lib/virtus/attribute/lazy_default.rb | 2 +- lib/virtus/attribute_set.rb | 2 +- spec/integration/default_values_spec.rb | 8 ++++++++ spec/unit/virtus/class_methods/new_spec.rb | 14 +++++++++++++- 5 files changed, 29 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 7f08fb66..03d284a8 100644 --- a/README.md +++ b/README.md @@ -157,6 +157,10 @@ class Page # default from a singleton value (boolean in this case) attribute :published, Boolean, :default => false + + # default from a singleton value (string in this case), + # that doesn't allow a nil value + attribute :author, String, :default => 'Mies', :allow_nil => false # default from a callable object (proc in this case) attribute :slug, String, :default => lambda { |page, attribute| page.title.downcase.gsub(' ', '-') } @@ -169,10 +173,11 @@ class Page end end -page = Page.new(:title => 'Virtus README') +page = Page.new(:title => 'Virtus README', :author => nil) page.slug # => 'virtus-readme' page.views # => 0 page.published # => false +page.author # => 'Mies' page.editor_title # => "UNPUBLISHED: Virtus README" page.views = 10 diff --git a/lib/virtus/attribute/lazy_default.rb b/lib/virtus/attribute/lazy_default.rb index 6363640b..112776a6 100644 --- a/lib/virtus/attribute/lazy_default.rb +++ b/lib/virtus/attribute/lazy_default.rb @@ -5,7 +5,7 @@ module LazyDefault # @api public def get(instance) - if instance.instance_variable_defined?(instance_variable_name) + if present?(instance) super else set_default_value(instance) diff --git a/lib/virtus/attribute_set.rb b/lib/virtus/attribute_set.rb index 66055a84..ac4fd797 100644 --- a/lib/virtus/attribute_set.rb +++ b/lib/virtus/attribute_set.rb @@ -209,7 +209,7 @@ def finalize # @api private def skip_default?(object, attribute) - attribute.lazy? || attribute.defined?(object) + attribute.lazy? || attribute.present?(object) end # Merge the attributes into the index diff --git a/spec/integration/default_values_spec.rb b/spec/integration/default_values_spec.rb index d9d9b1e8..91f7a2ec 100644 --- a/spec/integration/default_values_spec.rb +++ b/spec/integration/default_values_spec.rb @@ -15,6 +15,7 @@ class Page include Virtus attribute :title, String + attribute :author, String, :default => 'Mies', :allow_nil => false attribute :slug, String, :default => lambda { |post, attribute| post.title.downcase.gsub(' ', '-') }, :lazy => true attribute :view_count, Integer, :default => 0 attribute :published, Boolean, :default => false, :accessor => :private @@ -59,6 +60,13 @@ def default_editor_title end.to change { subject.view_count }.to(0) end + context 'when attribute is assigned as nil' do + specify 'you can set a default with the :allow_nil option' do + page = Examples::Page.new :author => nil + expect(page.author).to eql 'Mies' + end + end + context 'a ValueObject' do it 'does not duplicate the ValueObject' do page1 = Examples::Page.new diff --git a/spec/unit/virtus/class_methods/new_spec.rb b/spec/unit/virtus/class_methods/new_spec.rb index 63ca576b..15730834 100644 --- a/spec/unit/virtus/class_methods/new_spec.rb +++ b/spec/unit/virtus/class_methods/new_spec.rb @@ -7,7 +7,9 @@ attribute :id, Integer attribute :name, String, :default => 'John Doe' + attribute :alias, String, :default => 'jdoe', :allow_nil => false attribute :email, String, :default => 'john@doe.com', :lazy => true, :writer => :private + attribute :age, Integer, :default => 18, :allow_nil => false, :lazy => true } } @@ -16,21 +18,31 @@ it 'sets default values for non-lazy attributes' do expect(subject.instance_variable_get('@name')).to eql('John Doe') + expect(subject.instance_variable_get('@alias')).to eql('jdoe') end it 'skips setting default values for lazy attributes' do expect(subject.instance_variable_get('@email')).to be(nil) + expect(subject.instance_variable_get('@age')).to be(nil) end end context 'with attribute hash' do - subject { model.new(:id => 1, :name => 'Jane Doe') } + subject { model.new(:id => 1, :name => 'Jane Doe', :alias => nil, :age => nil) } it 'sets attributes with public writers' do expect(subject.id).to be(1) expect(subject.name).to eql('Jane Doe') end + it 'sets default values for attributes assigned as nil without allow_nil' do + expect(subject.alias).to eql('jdoe') + end + + it 'sets default values for lazy attributes assigned as nil without allow_nil' do + expect(subject.age).to eql(18) + end + it 'skips setting attributes with private writers' do expect(subject.instance_variable_get('@email')).to be(nil) end