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

Fix definition of serializer attributes with multiple calls to `attri… #1096

Merged
merged 1 commit into from
Sep 1, 2015

Conversation

beauby
Copy link
Contributor

@beauby beauby commented Aug 28, 2015

…buteinstead of one single call toattributes`.

Fixes #1094.

…bute` instead of one single call to `attributes`.
@@ -70,7 +70,7 @@ def self.attribute(attr, options = {})
ActiveModelSerializers.silence_warnings do
define_method key do
object.read_attribute_for_serialization(attr)
end unless respond_to?(key, false) || _fragmented.respond_to?(attr)
end unless (key != :id && method_defined?(key)) || _fragmented.respond_to?(attr)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this should be extracted to a helper method for readability

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're at it, should _fragmented.respond_to?(attr) not be _fragmented.respond_to?(key)? I haven't delved into the caching yet.

@bf4
Copy link
Member

bf4 commented Aug 28, 2015

👍

1 similar comment
@whatthewhat
Copy link
Contributor

👍

@joaomdmoura
Copy link
Member

Nice @beauby, I'm merging it, Tks! 😄

joaomdmoura added a commit that referenced this pull request Sep 1, 2015
Fix definition of serializer attributes with multiple calls to `attri…
@joaomdmoura joaomdmoura merged commit e0b74d8 into rails-api:master Sep 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants