Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #6322 (closed enhancement: fixed)

Opened 3 years ago

Last modified 11 months ago

[PATCH] cleanup composed_of and allow conversion block

Reported by: brandon Assigned to: bitsweat
Priority: normal Milestone: 2.x
Component: ActiveRecord Version: edge
Severity: normal Keywords: composed_of aggregations
Cc: bitsweat

Description

The attached patch cleans up the composed_of implementation by using define_method instead of just passing a string to module_eval. It also DRYs it up a bit.

This enables the second part of this patch, which adds a :conversion option to the composed_of declaration, which takes a Proc to convert the argument of the setter method into the expected type. This is useful for using composed_of fields in forms: 

composed_of :amount, :class_name => "Money", :conversion => Proc.new {|a| a.to_money }

Attachments

composed_of_conversion.diff (6.7 kB) - added by cch1 on 09/10/07 22:32:10.
Updated patch that accommodates the documentation changes made in revision 7368

Change History

10/01/06 01:21:58 changed by brandon

Uh, looks like the server is out of HD space: {{ OSError: [Errno 28] No space left on device: '/yumi/dev.rubyonrails.org/tracs/rails/attachments/ticket/6322' }}

The patch is available at http://opensoul.org/files/projects/composed_of_conversion.patch

10/01/06 03:48:39 changed by bitsweat

  • cc set to bitsweat.

Thanks for the patch. define_method leaks memory; that's the only reason why we eval. The conversion test fails for Nested::Class names - use class_name.constantize instead of Kernel.const_get. How about just taking a block instead of the Proc option: 

composed_of :amount, :class_name => 'Money' do |amount|
  Money.parse(amount)
end

10/01/06 04:09:56 changed by brandon

Thanks for the pointers. I like the idea of passing a block.

I didn't know that about the issues with define_method. Should I make the other changes in your comment and resubmit a patch, or is it ruled out due to the use of define_method? Do you have any idea how to use the block without using define_method?

10/01/06 05:07:50 changed by brandon

I've uploaded a new version to the URL above that uses a block instead of a Proc, replaces Kernel.const_get with class_name.constantize, and incorporates some small fixes from #5091.

It should also be noted that this implementation provides a solution for #5336.

10/01/06 05:11:13 changed by brandon

One more note. I noticed that define_method is used quite often in Rails, especially in the Active Record association code, which is used more heavily than composed_of.

10/09/06 01:25:31 changed by david

  • owner changed from David to rails@bitsweat.net.

03/16/07 21:08:22 changed by brandon

  • keywords changed from composed_of to composed_of aggregations.
  • summary changed from [PATCH] cleanup composed_of and allow :conversion Proc to [PATCH] cleanup composed_of and allow conversion block.

Any chance someone could look at accepting this? I've packaged it as a plugin (http://source.collectiveidea.com/public/rails/plugins/composed_of_conversion), but it would be great to have this in core.

03/17/07 00:18:02 changed by josh

  • keywords changed from composed_of aggregations to composed_of aggregations verified.
  • version set to edge.

06/25/07 19:42:41 changed by josh

  • keywords changed from composed_of aggregations verified to composed_of aggregations.
  • owner changed from rails@bitsweat.net to bitsweat.

09/10/07 21:53:29 changed by cch1

Sure would be nice to get this accepted... The patch fails after revision 7368.

09/10/07 22:32:10 changed by cch1

  • attachment composed_of_conversion.diff added.

Updated patch that accommodates the documentation changes made in revision 7368

10/22/07 12:56:12 changed by cch1

I've given up on this patch ever being accepted -perhaps it is too ambitious or perhaps its lack of tests have doomed it. In any case, I have opened a new ticket (#9843) which seeks only to fix the defects with composed_of when faced with nil values. I have also included failing tests on that patch.

Please consider closing this ticket in favor of #9843.

And I would GREATLY appreciate someone verifying my patch and tests on #9843.

10/23/07 17:39:39 changed by bitsweat

  • status changed from new to closed.
  • resolution set to fixed.

(In [8003]) Assigning an instance of a foreign class to a composed_of aggregate calls an optional conversion block. Refactor and simplify composed_of implementation. Closes #6322.