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

Delegate Serializer.attributes to Serializer.attribute #1167

Merged
merged 1 commit into from
Sep 18, 2015

Conversation

bf4
Copy link
Member

@bf4 bf4 commented Sep 17, 2015

No description provided.

define_method attr do
object && object.read_attribute_for_serialization(attr)
end unless method_defined?(attr) || _fragmented.respond_to?(attr)
attribute(attr)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

much cleaner!

@_attributes_keys[attr] = { key: key } if key != attr
@_attributes << key unless @_attributes.include?(key)
self._attributes_keys[attr] = { key: key } if key != attr
self._attributes << key unless self._attributes.include?(key)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this second use of _attributes violates the reference @beauby linked.... but I think it makes sense here.... so... :-\

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

what was fixed? (maybe I'm just missing it?)
self._attributes << key unless self._attributes.include?(key)
but, is this -----------------------------> ^ self supposed to be here?

Copy link
Member Author

Choose a reason for hiding this comment

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

n/m wrong pr.. juggling...

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed fureal

@NullVoxPopuli
Copy link
Contributor

needs rebase

@bf4 bf4 force-pushed the simplify_serializer_attributes branch from 0504ad5 to 27812a1 Compare September 17, 2015 19:49
@bf4 bf4 force-pushed the simplify_serializer_attributes branch from 27812a1 to 484426c Compare September 18, 2015 03:12
NullVoxPopuli added a commit that referenced this pull request Sep 18, 2015
Delegate Serializer.attributes to Serializer.attribute
@NullVoxPopuli NullVoxPopuli merged commit e0a0ad3 into master Sep 18, 2015
@bf4 bf4 deleted the simplify_serializer_attributes branch December 14, 2015 18:16
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.

3 participants