Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Replace AttributesAccessor with AttributeSet. #100

Merged
merged 1 commit into from Jul 3, 2012

Conversation

Projects
None yet
2 participants
Collaborator

emmanuel commented Jul 1, 2012

This way the methods for accessing a set of Attributes are together with those Attributes. The down side is that AttributeSet is marginally more complex. The upside is that Virtus::Extensions and Virtus::ClassMethods are simpler and more direct.

@emmanuel emmanuel Replace AttributesAccessor with AttributeSet.
This way the methods for accessing a set of
Attributes are together with those Attributes.
d606d65
Owner

solnic commented Jul 3, 2012

@emmanuel I can totally see the benefit coming from simplified extensions and classmethods. This doesn't change the fact I'm totally confused when I think that attribute set is now a module and a set of attributes :D I need to think about more

Collaborator

emmanuel commented Jul 3, 2012

Please do think about it more. I've convinced myself that this is a good idea, so I'm eager to hear other viewpoints.

I think of it like this: after this change, AttributeSet is a collection of Attribute instances, and the methods to use them. But, due to the way that modules work, these responsibilities don't interfere with each other at all. Until someone calls include attribute_set, the attribute accessor methods are totally out of the way.

Part of the reason I think this is an improvement is that, without this change, AttributeAccessor and AttributeSet are kept in sync externally, in ClassMethods and Extensions. It's not too bad now (ie., #virtus_add_attribute), but eg., removing an Attribute from an AttributeSet is currently a hassle.

Owner

solnic commented Jul 3, 2012

OK let's bring it in and we'll see what happens

@solnic solnic added a commit that referenced this pull request Jul 3, 2012

@solnic solnic Merge pull request #100 from emmanuel/attribute_set_as_module
Replace AttributesAccessor with AttributeSet.
da34fc8

@solnic solnic merged commit da34fc8 into solnic:master Jul 3, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment