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

refactor to make way for easy feature adding #1111

Closed

Conversation

NullVoxPopuli
Copy link
Contributor

Big methods can be hard to work with.

Since there are upcoming changes to the json adapter, I figured I'd make things a little easier to digest (especially re: recursion).

serializer.map { |s| FlattenJson.new(s).serializable_hash(options) }
end

# TODO: what is a virtual value?
Copy link
Member

Choose a reason for hiding this comment

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

it's an internal thing that uses that value instead of a serialized attribute. e.g. there's no serializer for Ruby arrays, so it would just use the array as the virtual value. see #962

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah ok, cool.

I'll add a comment about that to help future contributors.

@bf4
Copy link
Member

bf4 commented Aug 31, 2015

💯 Some thoughts:

  • I think all the new methods should be private so that we can change them with impunity, and would need to opt-in any we want to be part of the public interface
  • Would be good to take a look at the json api adapter for any common themes, but not required esp if all the new methods are private

thoughts?

@NullVoxPopuli
Copy link
Contributor Author

  • I think some of the methods could be private
  • There is def some overlap in functionality of some methods (at a high level) between these refactored json methods and the json_api methods.

I'll see what I can do to mimic json_api's naming and flow.

opts = association.options

if serializer.respond_to?(:each)
add_relationships(association.key, serializer, opts)
Copy link
Contributor

Choose a reason for hiding this comment

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

I know the terminology comes from the json_api adapter, but I think add_to_many_relationship and add_to_one_relationship are more intuitive name than the confusingly similar add_relationship/add_relationships.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, should json_api's naming change as well? - I am not as familiar with json_api though

@joaomdmoura
Copy link
Member

This is really nice. I'm closing it because it was already ported on #1114

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