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

Let FlattenJson adapter decide it doesn't include meta #1048

Merged
merged 1 commit into from
Aug 18, 2015

Conversation

bf4
Copy link
Member

@bf4 bf4 commented Aug 11, 2015

Why should the superclass get to decide it wants to
exclude meta from one of its subclasses. I say don't ask
if we're a FlattenJson adapter, tell the adapter to
include the meta and let FlattenJson no-op on it.

From discussion in #1041

@bf4 bf4 mentioned this pull request Aug 11, 2015
@bf4 bf4 changed the title Let FlattanJson adapter decide it doesn't include meta Let FlattenJson adapter decide it doesn't include meta Aug 12, 2015
@bacarini
Copy link
Contributor

👍

@bf4
Copy link
Member Author

bf4 commented Aug 13, 2015

@joaomdmoura any objections to moving the include_meta unless FlattenJson from the Adapter class to the FlattenJson class?


private

# no-op
Copy link
Member

Choose a reason for hiding this comment

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

What about a more specific comment, nothing big, just to help ppl jumping in the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

no-op: The FlattenJson adapter does not include meta data? no-op: The FlattenJson adapter only serializes attributes?

Copy link
Member

Choose a reason for hiding this comment

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

no-op: FlattenJson adapter does not include meta data, because it does not support root. 😄

@joaomdmoura
Copy link
Member

👍 LGTM, just waiting for the comment before merging.

@bf4
Copy link
Member Author

bf4 commented Aug 18, 2015

updated

@joaomdmoura
Copy link
Member

Great! I'm merging it.

joaomdmoura added a commit that referenced this pull request Aug 18, 2015
Let FlattenJson adapter decide it doesn't include meta
@joaomdmoura joaomdmoura merged commit e384b65 into rails-api:master Aug 18, 2015
@bf4 bf4 deleted the cleanup_meta_in_adapter branch August 18, 2015 19:23
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.

None yet

3 participants