solnic / virtus

[DISCONTINUED ] Attributes on Steroids for Plain Old Ruby Objects
MIT License
3.77k stars 229 forks source link

Fix coercion of enumerable types #146

Closed kawamanza closed 11 years ago

kawamanza commented 11 years ago

Coercion of enumerations (Arrays, Hashes and Sets) was not working. I made changes keeping the fallbacks. Please take a look.

Best regards.

dkubb commented 11 years ago

I'm not sure we would want to encourage subclassing primitives like that. It's almost always better to not subclass a built-in primitive class, and instead use composition to get the behaviour you want.

When you subclass something what you're saying is that you expect instances of your subclass to work anywhere that instances of the parent class are. Sometimes that's the case, but often you'll want to do something different and domain specific.

@solnic wdyt?

solnic commented 11 years ago

Yeah I agree I just used subclassing primitives in examples as it's...well, less code. I guess I should not do that since it's code that I would never write in real world.

kawamanza commented 11 years ago

Coercer works well with mass assignment, but when I've managed the enumeration by myself coercion doesn't work. Example:

class Thing
  include Virtus
  attribute :dimensions, Hash[Symbol => String]
end

thing = Thing.new(:dimensions => {"width" => 20})
thing.dimensions               # => {:width => "20"}
thing.dimensions["width"] = 10
thing.dimensions               # => {:width => "20", "width" => 10}
sowcow commented 11 years ago

also this example from readme doesn't work

class Book
  include Virtus

  attribute :title, String
end

class BookCollection < Array
  def <<(book)
   if book.kind_of?(Hash)
    super(Book.new(book))
   else
     super
   end
  end
end

class Library
  include Virtus

  attribute :books, BookCollection[Book]
end

library = Library.new
library.books << { :title => 'Another Introduction to Virtus' }
solnic commented 11 years ago

It's gonna be fixed in the next release. Fixes are already done in attribute-refactor branch that will be merged into master soon :smile:

sowcow commented 11 years ago

ok, thanks

solnic commented 11 years ago

This is now fixed in master