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

Support for modules with attributes #90

Merged
merged 23 commits into from Jun 8, 2012

Conversation

Projects
None yet
4 participants
Owner

solnic commented May 18, 2012

This is my initial work on issue #27

Contributor

apotonick commented on lib/virtus/module.rb in 5600671 May 18, 2012

Awesome, that's exactly how we do it in roar! Leeeeeeeet me see

Owner

solnic replied May 18, 2012

@apotonick ok great. I'll push 0.5.0 soon, probably during the weekend.

Collaborator

dkubb replied May 18, 2012

@solnic why reverse here? Also reverse_each creates one less intermediate array.

Contributor

apotonick commented on lib/virtus/module.rb in 5600671 May 18, 2012

Problem here is that Virtus still wants to be included in the class, which leads to problems when you extend an object with the coercion module! Here's how I do it in representable: https://github.com/apotonick/representable/blob/master/lib/representable.rb#L40

Owner

solnic replied May 18, 2012

OK now I'm lost :)

I think I don't understand what's your expectation from this feature. From my pov I wanted to be able to define attributes on a module, then include it in a class so that it's extended with Virtus and it gets all the attributes that were defined in the module.

Contributor

apotonick replied May 18, 2012

Here's how roar/representable works:

module SongRepresenter
  include Roar::Representer::JSON

  property :title
end

This module can be used in classes and instances.

Song.include(SongRepresenter).new.from_json ..

or

Song.new.extend(SongRepresenter).from_json

Oki?

Collaborator

dkubb replied May 18, 2012

@apotonick yeah, that's kind of how I expected this to work too. A module with attributes that can be mixed into other modules, classes and instances.

Owner

solnic replied May 18, 2012

so that's what I was missing. OK I'll work on that.

@ghost ghost assigned solnic May 18, 2012

Owner

solnic commented May 18, 2012

@dkubb am I missing something or is this all we need?

Collaborator

dkubb commented May 18, 2012

I wonder if it's possible to have one strategy that works for modules, classes and object instances without conditional logic? That would be the ideal case, but I don't see an obvious approach.

If conditionals are needed, perhaps we can break out what we have now (class based attributes) into Virtus::Class, and then have Virtus::Module handle the module/object instance case. Then when Virtus is included into something, it can choose the best strategy based on the type.

Collaborator

dkubb commented May 18, 2012

Whoops, above I meant that it should handle being "extended" into an object instance, not included. You probably knew what I meant, but I thought I should clarify.

@dkubb dkubb commented on an outdated diff Jun 4, 2012

spec/integration/using_modules_spec.rb
@@ -0,0 +1,43 @@
+require 'spec_helper'
+
+describe 'I can define attributes within a module' do
+ before do
+ module Examples
+ module Base
+ include Virtus
+
+ attribute :name, String
+ attribute :age, Integer
@dkubb

dkubb Jun 4, 2012

Collaborator

Does this also work with something like this:

module Examples
  module Name
    include Virtus

    attribute :name, String
  end

  module Age
    include Virtus

    attribute :age, Integer
  end

  class User
    include Name, Age
  end
end

What about if you mix in a virtus module into a superclass, then mix in another module into a subclass? I just want to make sure stuff doesn't overwrite each other, and that there's no limitation to how things can be mixed in.

solnic and others added some commits Jun 4, 2012

Rename ClassExtensions to ClassInclusions
* Change so that ClassInclusions is included, since it mostly just extends the
  descendant and adds instance methods.

@dkubb dkubb and 1 other commented on an outdated diff Jun 4, 2012

lib/virtus/instance_extensions.rb
+ # @api private
+ def allowed_writer_methods
+ @allowed_writer_methods ||=
+ begin
+ allowed_writer_methods = public_method_list.map(&:to_s)
+ allowed_writer_methods = allowed_writer_methods.grep(WRITER_METHOD_REGEXP).to_set
+ allowed_writer_methods -= INVALID_WRITER_METHODS
+ allowed_writer_methods.freeze
+ end
+ end
+ end
+
+ def self.extended(object)
+ object.extend(AllowedWriterMethods)
+ object.extend(InstanceMethods)
+ object.instance_eval do
@dkubb

dkubb Jun 4, 2012

Collaborator

@solnic do you think we can just do this here:

@virtus_attributes_accessor_module = AttributesAccessor.new(object.class.inspect)
object.extend @virtus_attributes_accessor_module
@solnic

solnic Jun 4, 2012

Owner

@dkubb we need that ivar, it's used in #attribute. so maybe object.instance_variable_set?

@dkubb

dkubb Jun 5, 2012

Collaborator

@solnic ahh yeah, after I mentioned it, I realized that the object needs the ivar set internally. I think #instance_eval the way we're doing it is probably nicer then.

Owner

solnic commented Jun 5, 2012

@apotonick hey dude I think I got it working with classes and instances. Could you check Roar against this branch and tell me if it works for you? TY!

Contributor

apotonick commented Jun 6, 2012

Ok this looks way better now! :) However, now if I use coercion in a module the accessor methods don't work:

module SongRepresenter
  include Representable::JSON
  include Representable::Coercion
  property :composed_at, :type => DateTime 
end

song = Song.new.extend(SongRepresenter).from_json("{\"composed_at\":\"November 18th, 1983\"}")
assert_kind_of DateTime, song.composed_at

NoMethodError: undefined method `composed_at=' for #<VirtusCoercionTest::Song:0xade9be4>

property just delegates to your stuff:

def property(name, args={})
  attribute(name, args[:type]) if args[:type]
Owner

solnic commented Jun 6, 2012

@apotonick ok I'll debug that. thanks for testing!

Contributor

apotonick commented Jun 6, 2012

Trick is to have a test which is just sitting in the test suite waiting to pass: https://github.com/apotonick/representable/blob/master/test/coercion_test.rb#L9 How do I have "pending" tests in MiniTest, BTW?

lgierth commented Jun 6, 2012

How do I have "pending" tests in MiniTest, BTW?

You need ActiveSupport or something else that extends MiniTest with pending semantics.

edit: I missed you at Euruko, btw :)

Owner

solnic commented Jun 7, 2012

@apotonick ok I made it pass but take a look at the TODO comment in the test: https://github.com/solnic/representable/commit/610b96fa3409608d602dc8ff83e3851f7eb87b91

solnic added a commit that referenced this pull request Jun 8, 2012

Merge pull request #90 from solnic/modules-spike
Support extending modules and instances 🎁

@solnic solnic merged commit 55ef818 into master Jun 8, 2012

Collaborator

dkubb commented on e9ed8d8 Jul 5, 2012

@solnic it looks like this broke the build. The "rake ci" task fails now.

Maybe we should look at making it so that "rake ci" runs on travis-ci? Then this kind of thing won't happen again because it will be obvious when code is committed without specs.

Owner

solnic replied Jul 5, 2012

@dkubb I know about that. planning to fix it soon

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