-
Notifications
You must be signed in to change notification settings - Fork 5
default option fix #20
base: master
Are you sure you want to change the base?
Conversation
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.
Save back-compatibility. It is important.
Imagine following scenarios
- Somebody used
writer: false
to disable auto-assignment of attribute which should be added inattributes
(I.e. he overrided reader) - Somebody used
reader: false
to prevent attribute from exposing intoattributes
@@ -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) |
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.
Incorrect semantic.
- Save
define_reader
(at least for back-compatibility) - Define
private_reader
or something - i think,
attribute :name, reader: :private
serves this case best
@@ -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) |
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.
Private reader should not be exposed. Also you are breaking back-comp here
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.
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.
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.
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.
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.
sorry, you are correct here
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.
forgot to request changes
|
fix #19