Skip to content
This repository

fixes composed_of for delegated attributes and methods #3807

Closed
wants to merge 1 commit into from

7 participants

Patrick Negri Sergey Nartimov José Valim Roman Shatsov Isaac Sanders Rafael Mendonça França Steve Klabnik
Patrick Negri

composed_of wasnt working for delegated attributes and methods again.

this is my first contribution, can any1 check if its valid?

thanks

Sergey Nartimov

It will be good to have test for it

José Valim
Owner

I am fine with this change but we do need to have a test for it, as @lest said.

Patrick Negri

did the tests, but (dont know why), the other tests are getting errors like "stack too deep", maybe i did forgot to do something prior to test preparation. but was not my fault, cant execute the aggregation tests on HEAD/rails too.

anyway, the tests for the delegate and methods are working.

José Valim
Owner

Ah, the stack too deep makes total sense. As the overridden method will probably invoked the composed_of bit.

Patrick Negri
Patrick Negri

Any tip on how to improve those tests?

Roman Shatsov

I made commit and now old and new tests pass. Can I attach code to this pull request without creating new?

Patrick Negri

roshats. Submit a Pull request to my Branch and i will update this pull.
Regards
Patrick

Isaac Sanders

Is this still an issue?

Patrick Negri
pnegri commented

dont know maybe we need to remerge again and pull

Rafael Mendonça França
Owner

@pnegri @roshats any news about this one?

Edward Tsech edtsech referenced this pull request from a commit in edtsech/rails
Edward Tsech Fix stack level to deep issue in #3807. 0448f6c
Patrick Negri pnegri closed this
Patrick Negri pnegri reopened this
Rafael Mendonça França
Owner

@pnegri could you squash your commits?

Patrick Negri pnegri referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Patrick Negri
pnegri commented

hey guys, any update on that?

Steve Klabnik
Collaborator

If #6743 gets merged, this feature won't be useful.

Patrick Negri pnegri referenced this pull request
Merged

Removing composed_of #6743

Steve Klabnik steveklabnik referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Steve Klabnik steveklabnik referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Steve Klabnik steveklabnik closed this pull request from a commit
Steve Klabnik Removing composed_of from ActiveRecord.
This feature adds a lot of complication to ActiveRecord for dubious
value. Let's talk about what it does currently:

class Customer < ActiveRecord::Base
  composed_of :balance, :class_name => "Money", :mapping => %w(balance amount)
end

Instead, you can do something like this:

    def balance
      @balance ||= Money.new(value, currency)
    end

    def balance=(balance)
      self[:value] = balance.value
      self[:currency] = balance.currency
      @balance = balance
    end

Since that's fairly easy code to write, and doesn't need anything
extra from the framework, if you use composed_of today, you'll
have to add accessors/mutators like that.

Closes #1436
Closes #2084
Closes #3807
14fc8b3
Steve Klabnik steveklabnik closed this in 14fc8b3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

May 28, 2012
Patrick Negri Fixes virtual attributes for composed_of and stack level too deep iss…
…ue in #3807
a2af5b5
This page is out of date. Refresh to see the latest.
12  activerecord/lib/active_record/aggregations.rb
@@ -220,17 +220,21 @@ def composed_of(part_id, options = {})
220 220
         constructor = options[:constructor] || :new
221 221
         converter   = options[:converter]
222 222
 
223  
-        reader_method(name, class_name, mapping, allow_nil, constructor)
  223
+        # Is name of method already used by composed_of ?
  224
+        method = mapping.length == 1 && mapping.first.first == name ?
  225
+          :read_attribute : :send
  226
+
  227
+        reader_method(name, class_name, mapping, allow_nil, constructor, method)
224 228
         writer_method(name, class_name, mapping, allow_nil, converter)
225 229
 
226 230
         create_reflection(:composed_of, part_id, options, self)
227 231
       end
228 232
 
229 233
       private
230  
-        def reader_method(name, class_name, mapping, allow_nil, constructor)
  234
+        def reader_method(name, class_name, mapping, allow_nil, constructor, method)
231 235
           define_method(name) do
232  
-            if @aggregation_cache[name].nil? && (!allow_nil || mapping.any? {|pair| !read_attribute(pair.first).nil? })
233  
-              attrs = mapping.collect {|pair| read_attribute(pair.first)}
  236
+            if @aggregation_cache[name].nil? && (!allow_nil || mapping.any? {|pair| !self.send(method, pair.first).nil? })
  237
+              attrs = mapping.collect {|pair| self.send(method, pair.first)}
234 238
               object = constructor.respond_to?(:call) ?
235 239
                 constructor.call(*attrs) :
236 240
                 class_name.constantize.send(constructor, *attrs)
56  activerecord/test/cases/aggregations_test.rb
@@ -156,3 +156,59 @@ def test_composed_of_aggregation_redefinition_reflections_should_differ_and_not_
156 156
                      DifferentPerson.reflect_on_aggregation(:composed_of)
157 157
   end
158 158
 end
  159
+
  160
+class MethodAndDelegationAggregationsTest <ActiveRecord::TestCase
  161
+
  162
+  class FullName
  163
+    attr_reader :fname, :lname
  164
+    def initialize(fname, lname)
  165
+      @fname, @lname = fname, lname
  166
+    end
  167
+
  168
+    def to_s
  169
+      @fname + " " + @lname
  170
+    end
  171
+
  172
+    def ==(other_name)
  173
+      return self.to_s == other_name if (other_name.kind_of? String)
  174
+      fname == other_name.fname && lname == other_name.lname
  175
+    end
  176
+  end
  177
+
  178
+  class DelegateTest
  179
+    def test_delegated
  180
+      "Boop"
  181
+    end
  182
+  end
  183
+
  184
+  class Person < ActiveRecord::Base
  185
+    def first_name
  186
+      "Bart"
  187
+    end
  188
+
  189
+    def first_name=
  190
+    end
  191
+
  192
+    def last_name
  193
+      "Simpsom"
  194
+    end
  195
+
  196
+    def last_name=
  197
+    end
  198
+
  199
+    composed_of :full_name, :class_name => "MethodAndDelegationAggregationsTest::FullName", :mapping => [ %W(first_name fname), %W(last_name lname) ]
  200
+
  201
+    attr_accessor :some_class
  202
+
  203
+    delegate :test_delegated, :to => :some_class
  204
+    composed_of :delegated_name, :class_name => "MethodAndDelegationAggregationsTest::FullName", :mapping => [ %W(first_name fname), %W(test_delegated lname) ]
  205
+  end
  206
+
  207
+  def test_composed_of_with_method_attribute
  208
+    person = Person.new
  209
+    assert_equal person.full_name, "Bart Simpsom"
  210
+    person.some_class =  MethodAndDelegationAggregationsTest::DelegateTest.new
  211
+    assert_equal person.delegated_name, "Bart Boop"
  212
+  end
  213
+
  214
+end
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.