solnic / virtus

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

Attributes now require a type #193

Closed cflipse closed 10 years ago

cflipse commented 10 years ago

Upgraded from beta0 to beta3 this morning, and the attributes method has started raising an Argument error. It seems that the type argument became required during one of the internal refactorings:

/Users/chrisflipse/src/photo_manager/vendor/cache/virtus/lib/virtus/module_builder.rb:109:in `block in attribute_method': wrong number of arguments (1 for 2) (ArgumentError)

The README doesn't give any indication that the type argument is not required anymore, but the documentation for the attributes method indicates that the argument is optional, defaulting to Object. I'm not sure if this is a documentation or implementation bug, but given that the are no deprication warnings, I'm guessing this was an accidental oversight?

solnic commented 10 years ago

Right. This is a regression. I kept that in the back of my head and eventually forgot to fix. I'll do it shortly.

dkubb commented 10 years ago

@cflipse according to @solnic's message a few hours ago, this is an intentional change: https://github.com/solnic/virtus/issues/161#issuecomment-24991855

I can't speak for the precise reasoning behind it, but I would assume it's because most of the time you do want to provide something beside Object, whether it be a String or Integer or something else. In my experience it's fairly rare that you have an object attribute that you don't know/care about the type.

I'm going to mark this as closed for now.

solnic commented 10 years ago

Hah bad sync :) I actually want to keep the backward compatibility here. Maybe it's something to change for 2.0. Re-opening...

solnic commented 10 years ago

...and btw it might be possible that sometimes you just want to annotate a model with some attributes w/o the need to provide types. Personally I never needed to do that but I suppose it could be useful.

solnic commented 10 years ago

@cflipse fixed in beta4

dkubb commented 10 years ago

Ok, sounds good. I think it is possible I that added the default to Object originally, or I asked for it as a feature request. My original thinking was it would be good for prototyping, like in DM how we don't require you to specify :length or :min and :max and use reasonable defaults. I still believe in production these options should be specified because it shows your intent more clearly to yourself or someone else who reads the code later.

cflipse commented 10 years ago

Updated, to beta4, but it looks like it's not entirely gone:

everything is fine in the following:

class Foo
  include Virtus

  attribute :bar
end

Buit, if I transition to the new Virtus.model as advised by the deprection warning:

class Foo
  include Virtus.model

  attribute :bar
end

I get: /Users/chrisflipse/src/photo_manager/.bundle/ruby/1.9.1/gems/virtus-1.0.0.beta4/lib/virtus/module_builder.rb:109:inblock in attribute_method': wrong number of arguments (1 for 2) (ArgumentError)`

solnic commented 10 years ago

@cflipse ok fixed in beta5. btw if you don't need types you probably want to turn coercion off too via include Virtus.model(:coerce => false)