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

MMRecordMarshaler: mergeDuplicateRecordResponseObjectDictionary #106

Open
ParmeshwarBayappu opened this issue Mar 16, 2015 · 1 comment
Open
Milestone

Comments

@ParmeshwarBayappu
Copy link

This is based on my understanding. I believe the purpose of this method is to merge the two dictionaries representing the same entity.
Potential Issues:

  1. While iterating over the primaryKeyPaths
    id dictionaryValue = [dictionary valueForKeyPath:keyPath];
    id protoRecordValue = [dictionary valueForKeyPath:keyPath]; //probably meant protoRecord.dictionary.

  2. The check for dictionariesContainIdenticalPrimaryKeys is probably unnecessary as the caller is expected to call this only in that case.

Enhancement Suggestion:

  1. I would suggest truly merging the two dictionaries (copy from dictionary to protoRecord.dictionary). Though I would generally expect this to have been the default behavior, since it should rarely be needed and you documented it very well, this could suggestion can be ignored.
@cnstoll cnstoll added this to the 1.4.2 milestone Jan 18, 2016
@cnstoll
Copy link
Contributor

cnstoll commented Jan 18, 2016

Good catch. I think you're right. There may be a bug there.

Currently there are no ongoing plans to ship a 1.4.2 milestone for MMRecord, but I am adding the issue to that module in any case just so we can have a clearer picture to the state of the framework.

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

No branches or pull requests

2 participants