Skip to content
This repository has been archived by the owner on Jun 2, 2021. It is now read-only.

default option fix #20

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions lib/tainbox/attribute_definer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def initialize(klass, attribute_name, requested_type, requested_args)
@requested_args = requested_args
end

def define_getter
def define_getter(public_access = true)
klass.tainbox_register_attribute(attribute_name)
attribute = attribute_name

Expand All @@ -22,23 +22,24 @@ def define_getter
value = instance_variable_get(:"@tainbox_#{attribute}")
value.is_a?(Tainbox::DeferredValue) ? instance_exec(&value.proc) : value
end
send(:private, attribute) unless public_access
end
end

def define_setter
def define_setter(public_access = true)
klass.tainbox_register_attribute(attribute_name)

attribute = attribute_name
args = requested_args
type = requested_type

klass.tainbox_layer.instance_eval do

define_method("#{attribute}=") do |value|
tainbox_register_attribute_provided(attribute)
value = Tainbox::TypeConverter.new(type, value, options: args).convert if type
instance_variable_set(:"@tainbox_#{attribute}", value)
end
send(:private, "#{attribute}=") unless public_access

define_method("tainbox_set_default_#{attribute}") do
if args.has_key?(:default)
Expand Down
4 changes: 2 additions & 2 deletions lib/tainbox/class_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ def attribute(name, type = nil, **args)
define_writer = args.fetch(:writer, true)

definer = Tainbox::AttributeDefiner.new(self, name, type, args)
definer.define_getter if define_reader
definer.define_setter if define_writer
definer.define_getter(define_reader)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect semantic.

  1. Save define_reader (at least for back-compatibility)
  2. Define private_reader or something
  3. i think, attribute :name, reader: :private serves this case best

definer.define_setter(define_writer)
end

def suppress_tainbox_initializer!
Expand Down
2 changes: 1 addition & 1 deletion lib/tainbox/instance_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def initialize(*args)

def attributes
self.class.tainbox_attributes.map do |attribute|
[attribute, send(attribute)] if respond_to?(attribute, true)
[attribute, send(attribute)] if respond_to?(attribute)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Private reader should not be exposed. Also you are breaking back-comp here

Copy link

@frvade frvade Apr 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Private reader should not be exposed

He is actually fixing this. Now only public readers will show up in attributes. But this will actually break backward-compatibility and needs to be discussed.

Copy link
Author

@loadkpi loadkpi Apr 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably it's better to compare #attributes behavior.
This code has similar results on master branch and on the pull request branch:

Class.new do
  include Tainbox
  attribute :a1
  attribute :a2, reader: false
  attribute :a3, writer: false
end.new.attributes
# => {:a1=>nil, :a3=>nil}

For me it's not really clear why respond_to? was used with the option to include private methods in its result before.
So let me know if I miss something.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, you are correct here

end.compact.to_h
end

Expand Down
41 changes: 41 additions & 0 deletions spec/tainbox_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,47 @@ def name
expect { person.new('Hello world') }.to raise_exception(ArgumentError, exception)
end

describe 'defining getter' do
let(:person) do
Class.new do
include Tainbox
attribute :name, default: 'Julyan', writer: false
end.new
end

it 'read the default value' do
expect(person.name).to eq('Julyan')
end

it 'does not allow to change the attribute value' do
expect { person.name = 'Adele' }.to raise_exception(NoMethodError)
end
end

describe 'defining setter' do
let(:person) do
Class.new do
include Tainbox
attribute :name, default: 'Julyan', reader: false
end.new
end

it 'has the default value' do
expect(person.send(:name)).to eq('Julyan')
end

it 'allows to change the attribute value' do
person.name = 'Adele'
expect(person.send(:name)).to eq('Adele')
end

describe '#attributes' do
it "doesn't contain defined attribute" do
expect(person.attributes).not_to have_key(:name)
end
end
end

describe 'string converter options' do

describe 'no options' do
Expand Down