karmajunkie / imperator

Imperator is a gem for creating small command objects
MIT License
256 stars 17 forks source link

Support `:attributes` to simplify multiple attribute definitions #6

Open nicolai86 opened 12 years ago

nicolai86 commented 12 years ago

Hey,

I'd like to be able to write

attributes :foo, :bar

instead of being forced to write

attribute :foo
attribute :bar

What steps would I need to take to add this to Imperator, @karmajunkie ?

Thanks for the nice gem!

statianzo commented 12 years ago

attribute comes from active_attr project. That would likely be the best place to add that functionality

karmajunkie commented 12 years ago

Yeah, the attribute method comes from the active_attr gem. I've been considering dropping that dependency and reimplementing a more limited version of it so I can eliminate the dependency on ActiveSupport 3.x, making it usable in environments where that gem is undesirable for one reason or another (such as rails 2 projects). Any interest in such a change?

nicolai86 commented 12 years ago

Yes, definitively - I for one would like to use this Gem in some legacy Rails 2.3 projects. If you like I'd provide a pull request.

statianzo commented 12 years ago

I came across Virtus on The Changelog today. It's an extraction of the DataMapper property API with only backports as a dependency. I created a virtus-spike branch that uses virtus instead of active_attr.

karmajunkie commented 12 years ago

@statianzo good find! I merged it in. I'd still like to pull out the activesupport dependency, which your branch doesn't seem to remove, but I still prefer the use of virtus to active_attr.

statianzo commented 12 years ago

I looked at that. To remove activesupport, you would also need to remove activemodel. That would require recreating callbacks and validations.

karmajunkie commented 12 years ago

yeah, i'm debating whether its worth doing that. If the dependency is removed, then the gem could at least be used on a Rails 2.3 project without validations and callbacks.

statianzo commented 12 years ago

If you're looking to do that, I would focus on the validations. Personally, I don't see the callbacks adding a lot of value.

On Mon, Jun 11, 2012 at 9:01 PM, Keith Gaddis < reply@reply.github.com

wrote:

yeah, i'm debating whether its worth doing that. If the dependency is removed, then the gem could at least be used on a Rails 2.3 project without validations and callbacks.


Reply to this email directly or view it on GitHub: https://github.com/karmajunkie/imperator/issues/6#issuecomment-6261859

nicolai86 commented 12 years ago

Callbacks can easily replaced by overwriting the method and calling super at the appropriate place. And we only need to support basic validations for now, since everything else can be added & tested as needed by the gem user.

At least to me validates_presence_of, validates_length_of, validates_{inclusion,exclusion}_of, validates_format_of and validates_acceptance_of is all that I need for basic validations...

karmajunkie commented 12 years ago

Yeah, I'm not really concerned about the callbacks. Supporting them activemodel-style with a class method and block is still fairly trivial.

The serialization support might be more complicated to support. I started working on that last night, and I'll probably finish it later this week. The serialization is important to have since its the key factor supporting the #commit method I want to add, but I don't to reinvent too much of the wheel. But if I can knock out the rest of that, then I can remove the activesupport dependency and roll a new release this weekend, and there shouldn't be any restrictions on where you could use it at that point.

karmajunkie commented 12 years ago

I added Issue #7 to track the removal of the activesupport dependency.

@leahpar with the virtus attribute implementation, it appears to require a type parameter with each attribute, which makes your request a bit more complicated to implement. I'd welcome a pull request but its not something I'm going to work on in the next few weeks. If and when I get to it, its likely to look something like this

class SomeClass < Imperator::Command
  attributes :one, :two, :three, :type => String  #possibility 1
  attributes :a => String, :b => Array            #possibility 2
end

Of the two, I think I prefer the latter syntax, though virtus supports several other options being supplied to the attribute. In that case, I think the values in the hash parameters would be optionally a hash of options to be supplied, but I think that might get ugly.

Thoughts?

statianzo commented 12 years ago

I prefer option 2. I'd say skip the optional nested hash. If you need something more complex than type declaration, then just use the attribute from virtus

nicolai86 commented 12 years ago

I'd also prefer option 2, but since we're now going the virtus route I'm not sure that the brevity is a gain anymore.

I've always compared attribute to attr_accessor and wanted to be able to use it the same way. But now I have to pass in a type for each attribute as well; which - at least to me right now - adds more work. And I wanted to reduce the amount of work I have to do ;)

What do we gain by adding a type to an attribute within the command pattern?

karmajunkie commented 12 years ago

Well, there's type coercion, which if you're using this in the context of a Rails controller is valuable, as you can just dump the params into the initializer and it will convert something like a string to an integer where necessary. More strategically it (using virtus) helps eliminate a dependency on activesupport. I'd prefer to require typed parameters (which are generally a good idea anyway) over syntactic sugar if the latter requires the activesupport dependency.

nicolai86 commented 12 years ago

When developing parts of my business logic I prefer duck typing since this makes things easier to test. I'm not sure that type coercion is a gain for the business logic which uses Imperator.

Are there any alternatives besides using virtus? Like a thin wrapper build around attr_accessor which takes care of the attribute handling?

On Jun 12, 2012, at 7:48 PM, Keith Gaddis wrote:

Well, there's type coercion, which if you're using this in the context of a Rails controller is valuable, as you can just dump the params into the initializer and it will convert something like a string to an integer where necessary. More strategically it helps eliminate a dependency on activesupport. I'd prefer to require typed parameters (which are generally a good idea anyway) over syntactic sugar if the latter requires the activesupport dependency.


Reply to this email directly or view it on GitHub: https://github.com/karmajunkie/imperator/issues/6#issuecomment-6277423

nicolai86 commented 12 years ago

I might back-paddle a little on this statement. Tried Imperator v0.2.0 some more the last couple of days and turns out with a little different coding style the type coercion is really nice.

kristianmandrup commented 12 years ago

I just issued a pull request to virtus in order to add this feature:

https://github.com/solnic/virtus/pull/109

Please update the Imperator README (and perhaps wiki) in order to be more useful with discussion of (at least one) scenario(s) where this Command pattern makes sense.