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

Added documentation to modify the hash by reference when working with serializeIntoHash #3660

Closed
wants to merge 1 commit into from

Conversation

rtablada
Copy link
Contributor

When working with serializeIntoHash if you want to modify the hash with a full set of other results, you are forced to either overwrite each individual key OR use something like _.extend to modify the hash argument in place using pass by reference.

This is a common use case when creating adapters for APIs that do not have a rootKey (though strongly discouraged).

I think this addition (or something to the effect should be added to make sure that this functionality is documented.

@@ -988,6 +988,7 @@ var JSONSerializer = Serializer.extend({
the payload and just sends the raw serialized JSON object.
If your server expects namespaced keys, you should consider using the RESTSerializer.
Otherwise you can override this method to customize how the record is added to the hash.
The hash property should be modified by reference (possibly using something like _.extend)
Copy link
Member

Choose a reason for hiding this comment

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

I like this addition to the docs but I'd prefer to see "(possibly using something like _.extend)" removed. "possibly using something like" doesn't fit the "voice" of the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, just a first pass. Let me commit and smash

@bmac
Copy link
Member

bmac commented Oct 12, 2015

ping @rtablada. Do you have time to update this pr?

@bmac
Copy link
Member

bmac commented Dec 4, 2015

This pr was merged as 63e9432

@bmac bmac closed this Dec 4, 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.

2 participants