Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add serializer to association block context #1633

Merged
merged 2 commits into from
Mar 30, 2016

Conversation

bf4
Copy link
Member

@bf4 bf4 commented Mar 30, 2016

Perhaps there some method on the serializer that I want to use in the association block.
I couldn't because it is being evaluated in the 'Reflection' object.

  • Add serializer to Reflection evaluation binding
  • Test that we can a serializer method from an association block
  • Document usage
  • Add changelog

@groyoh
Copy link
Member

groyoh commented Mar 30, 2016

Just as a proposal block_value = instance_exec(serializer, &block) would also be possible I guess and would be less "hidden".

@bf4
Copy link
Member Author

bf4 commented Mar 30, 2016

@groyoh oh yeah, I forgot you can do that

@bf4
Copy link
Member Author

bf4 commented Mar 30, 2016

oh, but then someone would need to add a block param.

-             has_many :roles do
+             has_many :roles do |serializer|
                meta count: object.posts.count
                serializer.cached_roles
              end

it's kind of nicer without that, right?

@bf4
Copy link
Member Author

bf4 commented Mar 30, 2016

diff --git a/lib/active_model/serializer/reflection.rb b/lib/active_model/serializer/reflection.rb
index 5257a90..1c70527 100644
--- a/lib/active_model/serializer/reflection.rb
+++ b/lib/active_model/serializer/reflection.rb
@@ -59,11 +59,9 @@ module ActiveModel
       def value(serializer)
         @object = serializer.object
         @scope = serializer.scope
-        # Add '@serializer' to binding for use in association block as 'serializer'
-        @serializer = serializer

         if block
-          block_value = instance_eval(&block)
+          block_value = instance_exec(serializer, &block)
           if block_value == :nil
             serializer.read_attribute_for_serialization(name)
           else
@@ -119,7 +117,7 @@ module ActiveModel

       protected

-      attr_accessor :object, :scope, :serializer
+      attr_accessor :object, :scope

       private

diff --git a/test/adapter/json_api/relationships_test.rb b/test/adapter/json_api/relationships_test.rb
index bf47a4e..5fa0de8 100644
--- a/test/adapter/json_api/relationships_test.rb
+++ b/test/adapter/json_api/relationships_test.rb
@@ -38,7 +38,7 @@ module ActiveModel
               end
             end

-            has_many :roles do
+            has_many :roles do |serializer|
               meta count: object.posts.count
               serializer.cached_roles
             end

@groyoh
Copy link
Member

groyoh commented Mar 30, 2016

Yeah, my point was that just using instance_eval "hides" the fact that we can use serializer (also object and scope). With instance_exec you know that you're using the parameter. And if you don't need to use serializer, you can simply not add any parameter to your block.

To me:

has_many :comments do |serializer|
  meta count: serializer.object.comments.count
end

is easier to understand than:

has_many :comments do
  meta count: object.comments.count
end

You don't need to look through the doc/code to know what are available within the block. But that's maybe just my opinion. And yes, the second version is a bit less verbose and nicer. 😉

In the end, I'm fine with any solution. I just wanted to point it out ;)

@bf4
Copy link
Member Author

bf4 commented Mar 30, 2016

@groyoh I like the idea. Added a commit

@groyoh
Copy link
Member

groyoh commented Mar 30, 2016

Why is appveyor taking so long?

@bf4
Copy link
Member Author

bf4 commented Mar 30, 2016

@groyoh done now

@bf4 bf4 merged commit 27a5de2 into rails-api:master Mar 30, 2016
@bf4 bf4 deleted the fix_reflection_block_context branch March 30, 2016 20:24
@bf4
Copy link
Member Author

bf4 commented Feb 21, 2017

Resolves #1976

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants